KKcorps commented on PR #19998:
URL: https://github.com/apache/pinot/pull/19998#issuecomment-6387216027
This comes from a model of upsert thread orders that StarTree runs on upsert
PRs, and it checked only the change to how the TTL watermark gets capped.
**The model's known bad orders replay the same, but a segment destroyed
inside the cap loop now turns off the out of TTL skip until the next round,
which opens one narrow duplicate.**
I replayed the model's 45 known bad orders with and without the change. A
search started inside the cap loop found this order on its own, and an hour of
wider search found nothing else.
**The order it adds.** With the skip off, an old segment N0 that comes back
ONLINE ignores its validDocIds snapshot and compares each row. Its A@0, which
lost to A@6 long ago, finds no key because metadataTTL already expired A, so
both rows are valid. It needs a destroy inside the cap loop and a re-add before
the next consumer starts, so it is rare, but it leaves a duplicate row.
```mermaid
sequenceDiagram
participant C as Consumer N3
participant R as Reload N1
participant M as Upsert manager
participant H as Helix N0
C->>M: takeSnapshot, N1 skipped while the reload holds its lock
R->>M: replaceSegment(N1), old N1 untracked
R->>R: old N1 destroyed
C->>M: getWatermarkToPersist, read lock on old N1 fails
M->>M: persist TTL_WATERMARK_NOT_SET
C->>M: removeExpiredPrimaryKeys drops A, its row A@6 in N2 stays valid
H->>M: N0 OFFLINE then ONLINE, addSegment(N0)
M->>M: no skip, so every N0 doc is compared
Note over M: A@0 finds no key, so A@0 and A@6 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'", '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'", 'add_committed_row:A@2')
('step', 'replace', "N1'", 'begin_leftover')
('step', 'replace', "N1'", 'untrack_and_offload')
('append', 'B', (5,), 3, {'v': 1}, False)
('append', 'A', (6,), 4, {'v': 1}, False)
('append', 'D', (9,), 5, {'v': 1}, False)
('assign_consumer', "N2'")
('acquire', "N2'")
('consume', "N2'", 3)
('consume', "N2'", 4)
('consume', "N2'", 5)
('commit_local', "N2'", 5)
('step', 'replace', "N2'", 'start_replace')
('step', 'replace', "N2'", 'add_committed_row:B@3')
('step', 'replace', "N2'", 'add_committed_row:A@4')
('step', 'replace', "N2'", 'add_committed_row:D@5')
('step', 'replace', "N2'", 'begin_leftover')
('step', 'replace', "N2'", 'untrack_and_offload')
('reload', 'N1')
('assign_consumer', "N3'")
('acquire', "N3'")
('step', 'reload', 'N1', 'start_replace')
('step', 'reload', 'N1', 'add_row:A@2')
('step', 'reload', 'N1', 'begin_leftover')
('step', 'reload', 'N1', 'untrack')
('destroy_segment', 4)
('step_watermark_cap',)
('sweep_expired_keys', ['A', 'C', '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', 'add_row:A@0')
('step', 'add_immutable', 'N0', 'add_row:C@1')
('step', 'add_immutable', 'N0', 'register')
```
</details>
Today's code does the same when N0 itself missed the round, and this window
also stops a late segment with no snapshot file from having every doc marked
valid, so it is a trade rather than a new kind of bug. Caching each segment's
max comparison value when it is added would keep today's behavior.
The model covers one replica and one partition, and it did not check the
mapped metadata sidecar.
<!-- upsert-model-check head=60040605eeb2ef6180fd8bf677df0c12dbfd3ef1 -->
--
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]
