Repository navigation
Conversation
9877c77 to
7d16389
Compare
7d16389 to
dae9817
Compare
dae9817 to
effefb7
Compare
effefb7 to
3e1dca0
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 14 of 14 changed files. Found one P2 move-accounting race that can trigger duplicate loads during inventory lag.
This is an automated review by Codex GPT-5.6-Luna(max)
| } else { | ||
| // Retain until the inventory view catches up, so the operation is never invisible in both. | ||
| holder.markAcknowledgedByServer(); | ||
| segmentsAwaitingConfirmation.put(holder.getSegment().getId(), holder); |
There was a problem hiding this comment.
[P2] Delay MOVE_TO callbacks until inventory confirmation
The holder is retained, but the following onRequestCompleted still dispatches its callback immediately. For a MOVE_TO, this removes the source MOVE_FROM mark and queues the source DROP before the target appears in inventory. If that DROP is also acknowledged while the source inventory still shows the segment, the next coordinator run sees DROP on the source and MOVE_TO on the target; replica accounting treats neither as a projected loaded replica, so a third eligible historical can receive an unnecessary duplicate load. Defer the move callback until target confirmation, or account for this acknowledged move pair explicitly.
|
This pull request has been marked as stale due to 60 days of inactivity. |
Fixes #18764.
Description
During move of a 1x-replicated segment from historical A to historical B, the coordinator can currently race and drop a segment temporarily from the timeline.
This occurs through 2 primary ways:
SegmentLoadQueueManager.moveSegmentoperation callsunmarkSegmentToDropand thendropSegmentwhich can race with readers.ServerInventoryView) and historical node callbacks (HttpLoadQueuePeon). Since these historical callbacks typically always arrive first, the code sums up load data from both sources and then does a diff. While this works if the HttpLoadQueuePeon is present in the queue (to cancel out the new pending load on historical B), before this fix, the peon deletes the holder the moment the server acks. So the DROP left the queue while the inventory still showed the segment loaded.Changes
SegmentMoveConcurrencyTest::testMarkToDropGraduatesToQueuedDrop).Release note
This PR has: