fix: add delayed segment removal to prevent load/drop race in Broker (#18738) - #20089
fix: add delayed segment removal to prevent load/drop race in Broker (#18738)#20089zhang-arvin wants to merge 2 commits into
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 3 of 3 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| // See https://github.com/apache/druid/issues/18738 | ||
| final ScheduledFuture<?> pendingRemoval = delayedRemovalExecutor.schedule( | ||
| () -> { | ||
| pendingSegmentRemovals.remove(segmentId); |
There was a problem hiding this comment.
[P1] Stale timer can remove a newer pending removal
An expired timer removes the segment ID without verifying that its future is still current. If it loses the lock to a reload followed by another drop, the newer timer is installed, then the older task can pass the selector checks and remove the timeline entry before the newer delay expires, reintroducing the load/drop race and causing partial query results. Guard removal with the expected future or a generation token under the same lock.
Use ConcurrentHashMap.remove(key, value) to atomically verify that the pending removal future is still current before removing the segment from the timeline. This prevents a stale timer from removing a newer pending removal entry when a segment is dropped, reloaded, and dropped again in quick succession. Addresses P1 review comment from FrankChen021 on PR apache#20089.
|
@FrankChen021 Thanks for the review! Fixed the P1 issue in 653beaa: the timer callback now uses ConcurrentHashMap.remove(segmentId, pendingRemoval) to atomically verify the future is still current before removing the timeline entry. |
hmm, drive by comment (haven't really reviewed code changes) but wonder is this good? presumably a lot more pure drops happen than re-loads to the same server, won't this result in more queries getting missing results because we didn't remove a server that has dropped the segment? |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 1 |
| P3 | 0 |
| Total | 3 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 1 |
| P3 | 0 |
| Total | 3 |
Reviewed 3 of 3 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| // See https://github.com/apache/druid/issues/18738 | ||
| final ScheduledFuture<?> pendingRemoval = delayedRemovalExecutor.schedule( | ||
| () -> { | ||
| if (pendingSegmentRemovals.remove(segmentId, pendingRemoval)) { |
There was a problem hiding this comment.
[P1] Stale timer is not rechecked under lock
The callback removes its future from pendingSegmentRemovals before taking the timeline lock. A segment can be re-added in that gap and install a newer delayed removal; the old callback can then remove the segment while the newer callback sees a pending entry and returns, leaving the freshly re-added segment absent from the timeline. Recheck that the callback still owns the current pending future under the same lock, or make replacement and removal atomic.
| "Asked to remove timeline entry[interval: %s, version: %s] that doesn't exist", | ||
| segment.getInterval(), | ||
| segment.getVersion() | ||
| final long delayMillis = segmentWatcherConfig.getSegmentDropDelayMillis(); |
There was a problem hiding this comment.
[P1] Delayed drops expose empty selectors to queries
During the delay, the empty ServerSelector remains in the timeline. CachingClusteredClient can include that holder, pick() returns null, and groupSegmentsByServer omits the segment while computeUncoveredIntervals still sees the timeline holder, allowing a partial result without an uncovered interval. Remove or mark the holder unavailable during the delay, or make query-side uncovered-interval handling treat an empty selector as uncovered.
| delayMillis, | ||
| TimeUnit.MILLISECONDS | ||
| ); | ||
| final ScheduledFuture<?> previous = pendingSegmentRemovals.put(segmentId, pendingRemoval); |
There was a problem hiding this comment.
[P2] Timer can fire before pending state is published
The scheduled callback can run before pendingSegmentRemovals.put completes. It then finds no pending entry and returns, after which the future is published with no callback left to remove the segment from the timeline. Publish the pending state before scheduling, or synchronize publication with callback execution.
|
Thanks @clintropolis for the feedback on the delay-removal approach. The scheduled executor with cancellation on reload was chosen to avoid the race condition where a segment is dropped while a new load is in progress. Happy to discuss alternative approaches. |
…8738) Addresses review comments from FrankChen021 and clintropolis on PR apache#20089. P1 fix: timer is now rechecked under lock to prevent stale timer from removing a newer pending removal entry. The synchronized(lock) block wraps the remove(key, value) check, ensuring the lock is held during both the future verification and the selector cleanup. P1 fix: empty selectors are no longer exposed to queries during the delay window. The segment is removed from the timeline immediately when the last server is dropped, while the selector is kept in the selectors map. If a new server announces the segment during the delay, it is re-added to the timeline. Only the selector cleanup is delayed. P2 fix: handle race where timer fires before pendingSegmentRemovals.put() completes. After the put, we check pendingRemoval.isDone() and run the cleanup inline if the timer already fired. Architecture response to clintropolis: The segment is now removed from the timeline immediately, so pure drops are not affected by the delay — the timeline entry is gone right away as before. The delay only applies to the cleanup of the selector from the selectors map, which allows a new server to re-add the segment to the timeline during the delay window. This preserves the race-condition fix while avoiding the problem of empty selectors in the timeline during the delay. Addresses PR comments: - P1: stale timer not rechecked under lock - P1: delayed drops expose empty selectors to queries - P2: timer can fire before pending state is published
653beaa to
d28a1a8
Compare
|
Thanks @clintropolis and @FrankChen021 for the feedback! I've addressed the issues:
The fix is pushed — please re-review when you have time. |
3 participants
Description
Fixes the segment load/drop race condition in the Broker (issue #18738).
When a segment is dropped and immediately reloaded, the Broker's segment watcher can incorrectly remove the newly loaded segment from its view. This happens because the segment removal is triggered by the drop announcement, but the reload announcement may arrive before the removal is actually processed.
Changes
segmentRemovalDelayMsconfiguration to delay segment removal after a drop announcement, allowing time for reload announcements to cancel the removal.Testing