KKcorps commented on PR #19998:
URL: https://github.com/apache/pinot/pull/19998#issuecomment-6437148363
**This closes the order from the last comment. The model found one much
narrower path to the same duplicate, and today's code reaches that duplicate
through another order too.**
None of the model's 45 known bad orders changed. An hour of search over 8
table shapes, plus a search started next to a destroyed segment, found only
this order.
**The order that is left.** A Helix thread publishes N2's marker and stalls
before reading the max. A reload then destroys the old N2, but the cap loop's
iterator still returns its entry, so the lock fails and the round persists no
watermark. The stall has to span the whole replace, so this is far rarer than
the last order.
```mermaid
sequenceDiagram
participant T as Helix add N1
participant R as Helix reload N2
participant C as Consumer N4
participant M as Upsert manager
participant H as Helix N0
T->>M: A@4 beats A@2 in N2, putIfAbsent(N2, NOT_SET)
R->>M: hold N2's segment lock
C->>M: snapshot skips N2, cap iterator reaches the old N2 entry
R->>M: replaceSegment(N2), untrackSegment(old N2)
R->>M: old N2 destroyed
C->>M: entry is NOT_SET, tryAcquireReadLock(old N2) fails
M->>M: persist TTL_WATERMARK_NOT_SET
T->>M: tryAcquireReadLock(old N2) fails, return
C->>M: removeExpiredPrimaryKeys drops A, its row A@4 in N1 stays valid
H->>M: N0 OFFLINE then ONLINE, addSegment(N0) in full
Note over M: A@0 finds no key, so A@0 and A@4 are both valid
```
<details><summary>Model history</summary>
```
('append', 'A', (0,), 0, {'v': 1}, False)
('append', 'C', (0,), 1, {'v': 1}, False)
('assign_consumer', "N0'")
('acquire', "N0'")
('consume', "N0'", 0)
('consume', "N0'", 1)
('commit_local', "N0'", 1)
('step', 'replace', "N0'", 'start_replace')
('step', 'replace', "N0'", 'read_marked_max')
('step', 'replace', "N0'", 'add_committed_row:A@0')
('step', 'replace', "N0'", 'add_committed_row:C@1')
('step', 'replace', "N0'", 'begin_leftover')
('step', 'replace', "N0'", 'untrack_and_offload')
('append', 'A', (4,), 2, {'v': 1}, False)
('assign_consumer', "N1'")
('acquire', "N1'")
('consume', "N1'", 2)
('commit_local', "N1'", 2)
('step', 'replace', "N1'", 'start_replace')
('step', 'replace', "N1'", 'read_marked_max')
('step', 'replace', "N1'", 'add_committed_row:A@2')
('step', 'replace', "N1'", 'begin_leftover')
('step', 'replace', "N1'", 'untrack_and_offload')
('offline_immutable', 'N1')
('step', 'offline', 'N1', 'begin_remove')
('step', 'offline', 'N1', 'remove:A')
('step', 'offline', 'N1', 'untrack')
('append', 'A', (2,), 3, {'v': 1}, False)
('append', 'B', (5,), 4, {'v': 1}, False)
('assign_consumer', "N2'")
('acquire', "N2'")
('consume', "N2'", 3)
('consume', "N2'", 4)
('commit_local', "N2'", 4)
('step', 'replace', "N2'", 'start_replace')
('step', 'replace', "N2'", 'read_marked_max')
('step', 'replace', "N2'", 'add_committed_row:A@3')
('step', 'replace', "N2'", 'add_committed_row:B@4')
('step', 'replace', "N2'", 'begin_leftover')
('step', 'replace', "N2'", 'untrack_and_offload')
('append', 'D', (9,), 5, {'v': 1}, False)
('assign_consumer', "N3'")
('acquire', "N3'")
('consume', "N3'", 5)
('commit_local', "N3'", 5)
('step', 'replace', "N3'", 'start_replace')
('step', 'replace', "N3'", 'read_marked_max')
('step', 'replace', "N3'", 'add_committed_row:D@5')
('step', 'replace', "N3'", 'begin_leftover')
('step', 'replace', "N3'", 'untrack_and_offload')
('add_immutable', 'N1')
('step', 'add_immutable', 'N1', 'start_add')
('step', 'add_immutable', 'N1', 'read_marked_max')
('step', 'add_immutable', 'N1', 'add_row:A@2')
('reload', 'N2')
('assign_consumer', "N4'")
('acquire', "N4'")
('step', 'reload', 'N2', 'start_replace')
('step', 'reload', 'N2', 'read_marked_max')
('step', 'reload', 'N2', 'add_row:A@3')
('step', 'reload', 'N2', 'add_row:B@4')
('step', 'reload', 'N2', 'begin_leftover')
('step', 'reload', 'N2', 'untrack')
('destroy_segment', 6)
('step_watermark_cap',)
('step_watermark_cap',)
('step', 'add_immutable', 'N1', 'read_marked_max')
('step', 'add_immutable', 'N1', 'register')
('sweep_expired_keys', ['C', 'A', 'B'])
('offline_immutable', 'N0')
('step', 'offline', 'N0', 'begin_remove')
('step', 'offline', 'N0', 'skip_out_of_ttl')
('step', 'offline', 'N0', 'untrack')
('add_immutable', 'N0')
('step', 'add_immutable', 'N0', 'start_add')
('step', 'add_immutable', 'N0', 'read_marked_max')
('step', 'add_immutable', 'N0', 'add_row:A@0')
('step', 'add_immutable', 'N0', 'add_row:C@1')
('step', 'add_immutable', 'N0', 'register')
```
</details>
Keeping the comparison column's max on heap, as the old metadata did, would
close this. It would also close a Helix thread marking a segment already
removed out of TTL, which the model cannot build.
The model covers one replica and one partition, and not the mapped metadata
sidecar.
<!-- upsert-model-check head=7cc009c556ca63a889901d22621fac41ad589d00 -->
--
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]
