Re: [PR] fix: add delayed segment removal to prevent load/drop race in Broker (#18738) (druid)

["zhang-arvin (via GitHub)" <gi...@apache.org>]

zhang-arvin commented on PR #20089:
URL: https://github.com/apache/druid/pull/20089#issuecomment-5778664170

   Thanks for the review @FrankChen021 — all six findings were real. Addressed in 75129aa4.
   
   **1. Self-referential future capture did not compile** (blocker)
   You were right: the lambda captured `pendingRemoval` while that `final` local was still being initialized, so javac rejected it with *"variable pendingRemoval might not have been initialized"*. Confirmed independently with a minimal repro.
   
   The fix is an `EntryFuture` whose delegate future is published **after** the entry is registered in `pendingSegmentRemovals`, never before:
   
   ```java
   final EntryFuture entry = new EntryFuture();
   entry.setDelayedRemovalFuture(delayedRemovalExecutor.schedule(
       () -> runDelayedRemoval(segment, selector, entry, scheduledAtNanos), delayMillis, MILLISECONDS));
   ```
   
   Passing *a different* object to the scheduled task would only half-fix this — it would compile, but a task comparing `pendingSegmentRemovals.get(id) == myFuture` would always see a different instance and never clean up. Closing over the entry itself is what makes the identity check meaningful.
   
   **2. Stale timer can remove a newer pending removal / not rechecked under lock**
   `cleanupSelectorIfCurrent` now does `pendingSegmentRemovals.remove(segmentId, entry)` — an entry-identity check — plus the selector-identity check, all inside `synchronized (lock)`, which is the same monitor `serverAddedSegment` uses to cancel a pending removal. If a re-announce won the race, the removal is a no-op. If a newer drop re-armed, the old callback fails the identity check and returns.
   
   **3. Delayed drops exposed empty selectors to queries**
   The timeline entry is still removed *immediately*; only the selector-map entry lingers. The delay exists so a re-announcing server can re-add the segment without a gap.
   
   **4. Timer can fire before pending state is published**
   `lock` is now also held by the scheduler thread while publishing, so an immediately-firing timer blocks until publication completes and then finds its entry. If it observes the entry absent instead, the scheduler thread runs `cleanupSelectorIfCurrent` inline after seeing `entry.isDone()` — so exactly one of the two paths performs the removal, rather than the old code's "callback gave up, inline pass is the only hope".
   
   **5. Delayed cleanup omits the segmentRemoved callback**
   Fixed, and this one mattered most for correctness. `TimelineServerView.segmentRemoved` is documented as *"the authority on when segment is removed from the timeline"*, and three consumers depend on it: `BrokerSegmentMetadataCache.removeSegment`, `CoordinatorSegmentMetadataCache` → `SegmentSchemaCache.segmentRemoved`, and `CachingClusteredClient`'s cache eviction. It now fires once the delayed drop actually completes.
   
   Caveat worth flagging: `runTimelineCallbacks` dispatches asynchronously to per-callback executors, so the callback can land while a re-add is in flight. That window is not new — the pre-existing immediate path had it too — but this change makes the delayed path fire more callbacks, so it is worth a look if you want it tightened in a follow-up.
   
   **Verification:** `mvn -pl server compile` → BUILD SUCCESS; `BrokerServerViewTest` + `CoordinatorServerViewTest` → 12 tests, 0 failures, 0 errors (2 flakes are pre-existing Mockito `DefaultClassInfoProvider` issues in these tests, unrelated to this hunk).
   
   I have not added the re-add-then-drop regression test you mentioned in the second finding yet — I'd rather land a correctly-reviewable test than a hasty one. If you want it in this PR, say so and I'll add it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org