KKcorps commented on PR #19088:
URL: https://github.com/apache/pinot/pull/19088#issuecomment-6335924523
This comes from a model of upsert thread orders that StarTree runs on upsert
PRs.
**On a `dropOutOfOrderRecord` table, this adds one new bad order, where a
key owns a doc that `index()` never writes. It clears none of the model's known
ones.**
I replayed the 45 histories behind the model's known bad orders and none
changed. Then I searched 12 `dropOutOfOrderRecord` table shapes for 61 minutes.
The search found this order 14 times, never on today's code.
**The new order.** `isOutOfOrderRecord()` reads the key without its lock,
and the drop branch still calls `addRecord()` with docId `numDocsIndexed`. If a
Helix thread takes the key's segment OFFLINE in between, `addRecord()` sees a
new key and marks that unwritten doc valid, so the next record lands in it. It
needs a segment reset at that exact moment, so it is rare.
```mermaid
sequenceDiagram
participant C as Consumer N1
participant M as Upsert metadata
participant H as Helix thread
Note over M: A points at N0, comparison 5
C->>M: index(A, comparison 3), the peek says out of order
H->>M: N0 ONLINE to OFFLINE, removeSegment drops A
C->>M: drop branch addRecord(A, docId 0) finds no A, marks doc 0 valid
Note over C: doc 0 is never written, numDocsIndexed stays 0
C->>M: index(B, value 7) writes doc 0, so A and B share it
C->>M: index(A, comparison 6) merges with doc 0, which is B's row
C->>M: addRecord(A) moves A to doc 1 and clears doc 0
Note over M: A's value is 8 instead of 2, and B is gone from queries
```
<details><summary>Model history</summary>
```
('append', 'A', (5,), 0, {'v': 1}, False)
('append', 'A', (3,), 1, {'v': 1}, False)
('append', 'B', (1,), 2, {'v': 7}, False)
('append', 'A', (6,), 3, {'v': 1}, False)
('assign_consumer', "N0'")
('acquire', "N0'")
('consume', "N0'", 0)
('finish_index', "N0'", 0)
('commit_local', "N0'", 0)
('step', 'replace', "N0'", 'start_replace')
('step', 'replace', "N0'", 'add_committed_row:A@0')
('step', 'replace', "N0'", 'begin_leftover')
('step', 'replace', "N0'", 'untrack_and_offload')
('assign_consumer', "N1'")
('acquire', "N1'")
('consume', "N1'", 1)
('offline_immutable', 'N0')
('step', 'offline', 'N0', 'begin_remove')
('step', 'offline', 'N0', 'remove:A')
('step', 'offline', 'N0', 'untrack')
('finish_index', "N1'", 1)
('consume', "N1'", 2)
('finish_index', "N1'", 2)
('consume', "N1'", 3)
('finish_index', "N1'", 3)
('add_immutable', 'N0')
('step', 'add_immutable', 'N0', 'start_add')
('step', 'add_immutable', 'N0', 'add_row:A@0')
('step', 'add_immutable', 'N0', 'register')
```
</details>
Metering the drop without calling `addRecord()` would close it.
The model has no indexing failures or dedup, so the rest of this PR was not
checked.
<!-- upsert-model-check head=b46dd0c4490fa4469bda91649e6545075f88534f -->
--
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]
