KKcorps commented on PR #19817:
URL: https://github.com/apache/pinot/pull/19817#issuecomment-6141673810
The semaphore half holds up in the model: a replace that reverts now keeps
the slot, so the next consuming segment can't merge against a key the revert is
about to move back. The pauseless half adds a counterexample. Excluding
pauseless tables from `isTableTypeInconsistentDuringConsumption()` takes the
PROTECTED revert away from them, and a reset of a pauseless consuming segment
then loses a merged value.
**The order that breaks.** This is a pauseless partial-upsert table in
PROTECTED mode with an INCREMENT column.
1. Committed segment `N0` is loaded on the server. It holds key `A` with
comparison value 0 and value 1.
2. Consuming segment `N1` reads a newer row for `A` (comparison 1, +1) and
merges it to 2. `doAddRecord` moves `A` off `N0`. Before this PR it also
records `N0` as `A`'s previous location, and with the PR it records nothing.
3. `N1` is reset, for example CONSUMING to OFFLINE after a consumer error.
`doOffload` calls `removeSegment`, and since
`shouldRevertMetadataOnInconsistency` is now false, the key for `A` is deleted
instead of pointed back at `N0`.
4. The new consumer for `N1` replays the same row. `A` has no key, so the
partial merge treats it as new and stores 1.
A replica that consumed every offset once holds 2, and this one holds 1.
Before the PR, step 3 restores `A` to `N0` and the replay gives 2. A pauseless
replica that consumed past its committed offset loses its base the same way,
because its download now removes the leftover rows instead of reverting them,
and in one search the key disappeared entirely. The previous-location map only
stores immutable locations, so in both orders the revert target is a loaded
committed segment.
The description also doesn't mention that `getParallelConsumptionPolicy`
reads the same method. A pauseless partial-upsert table that leaves the policy
unset now derives `ALLOW_ALWAYS` instead of `ALLOW_DURING_BUILD_ONLY`, so the
next segment can also consume during a download and re-merge a row the
over-consumed segment already folded.
**What was checked.** I checked this against a Python model of upsert
handoff that searches thread orders with Hypothesis. I replayed the 45 pinned
findings with the change off and on, and none changed. The pinned pauseless
histories, a steal during the build and a mismatched pauseless build, still
break with it. The comparison search ran for 62 minutes, 578 searches over 9
seeds at 1,500 histories each. It covered the default handoff, restarts,
rebalance, OFFLINE, deletes, metadataTTL, multi-column comparison, SNAPSHOT
queries, async removal and minion tasks, plus a pauseless table and a PROTECTED
partial table with `allowPartialUpsertConsumptionDuringCommit` so the slot is
released early. It found the over-consumed order above. Five pauseless
witnesses that break today come out clean with the change, because there a
revert restores a key into a segment that is already being removed, and
pauseless tables no longer revert. Pauseless searches mostly stop early on
older pauseless fa
ilures, so I also ran a 16-minute search that starts from a loaded committed
segment and a running consumer. It found the reset order in 15 histories that
are correct without the change. With the early-release table, the order where
the next segment merges before the revert breaks today and is correct with the
change. A clean search only means these searches didn't find a break.
The model covers one replica's handoff of one partition, not the whole
server, the controller or the stream.
<details>
<summary>Raw history the search found (pauseless, change on)</summary>
```
('append', 'A', (0,), 0, {'v': 1}, False)
('append', 'B', (0,), 1, {'v': 1}, False)
('assign_consumer', "N0'")
('acquire', "N0'")
('consume', "N0'", 0)
('consume', "N0'", 1)
('commit_local', "N0'", 1)
('step', 'replace', "N0'", 'release_semaphore_early')
('step', 'replace', "N0'", 'start_replace')
('step', 'replace', "N0'", 'add_committed_row:A@0')
('step', 'replace', "N0'", 'add_committed_row:B@1')
('step', 'replace', "N0'", 'begin_leftover')
('step', 'replace', "N0'", 'untrack_and_offload')
('assign_consumer', "N1'")
('acquire', "N1'")
('offload', "N1'")
('append', 'A', (1,), 2, {'v': 1}, False)
('append', 'A', (2,), 3, {'v': 1}, False)
('append', 'A', (3,), 4, {'v': 1}, False)
('append', 'A', (4,), 5, {'v': 1}, False)
('consume', "N1'", 2)
('step', 'offload', "N1'", 'stop')
('step', 'offload', "N1'", 'close_consumer')
('step', 'offload', "N1'", 'begin_remove')
('step', 'offload', "N1'", 'walk:A')
('step', 'offload', "N1'", 'finish_revert')
('step', 'offload', "N1'", 'release_semaphore')
('step', 'offload', "N1'", 'untrack')
('assign_consumer', "N1'r1")
('acquire', "N1'r1")
('consume', "N1'r1", 2)
```
</details>
<!-- theseus-pr-check head=6d027134a42e696b0c499c6736c8950dbae96a81 -->
--
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]
