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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
