deepthi912 opened a new pull request, #19794:
URL: https://github.com/apache/pinot/pull/19794
## Problem
When a consuming segment is replaced by a downloaded copy, the replacement
ends at the offset recorded in ZK. A replica that consumed *past* that offset
owns primary keys for rows the replacement does not contain, and those record
locations get rewritten as the replace lands. If the next consuming segment of
the partition is already running, it replays against that half-updated metadata.
`ParallelSegmentConsumptionPolicy` decides this per table, statically. For
upsert tables the only safe setting is `DISALLOW_ALWAYS`, which stalls the next
consuming segment on **every** commit — even though over-consumption is the
rare case. The existing code already notices the hazard and can only log about
it:
```java
_segmentLogger.error("Next consuming segment can run during download while
this replace reverts upsert "
+ "metadata. Its snapshot can miss the rows the revert restores. Use
DISALLOW_ALWAYS");
```
## Change
Decide per transition instead of per table. The offset that distinguishes
the two cases is already in hand — `downloadSegmentAndReplace()` receives the
committed `SegmentZKMetadata`, so comparing `getEndOffset()` against
`_currentOffset` costs nothing and needs no extra ZK read.
| `_currentOffset` vs committed `endOffset` | Next consuming segment |
|---|---|
| **greater** — consumed past the commit | **held** until the replace
completes |
| equal | released, as before |
| less | released, as before |
This applies to any upsert table; it does not look at upsert mode,
consistency mode, or CRC.
`doOffload()` still releases the semaphore afterwards, in a `finally`, after
removing this segment's metadata — so holding it here leaks nothing, and it
closes the stream consumer on that path too.
When the build path already released the semaphore there is nothing left to
hold back, so that case keeps the existing error log and
`UPSERT_REVERT_WITH_CONSUMPTION_DURING_DOWNLOAD` metric.
## Effect
- An upsert table that **did not** over-consume now releases even in
`DISALLOW_ALWAYS`-worthy configurations — today it stalls on every commit.
- An upsert table that **did** over-consume now holds even under
`ALLOW_ALWAYS` — today it races.
## Open question for reviewers
The predicate is deliberately offset-only. `downloadSegmentAndReplace()` is
also reached when the local build failed or mismatched CRC, in `DOWNLOAD`
completion mode, and from `DISCARDED`/`ERROR` — all with offsets equal. Those
keep releasing the semaphore, on the basis that an equal offset range means the
same rows.
The existing
`testDownloadAfterLocalBuildFlagsUpsertRevertWhenConsumingDuringBuild` covered
the CRC-mismatch case at equal offsets and expected the metric to fire; it now
expects the opposite. **If a CRC mismatch at an equal offset range can change
the row set, this predicate is too narrow and should widen.** Flagging it
explicitly rather than burying it in a test diff.
## Testing
`RealtimeSegmentDataManagerTest` — 35/35 pass. New
`testDownloadHoldsConsumptionWhenConsumedPastCommittedEndOffset` covers
over-consumed → held, equal → released, and
over-consumed-with-semaphore-already-released → flagged. `checkstyle` and
`spotless` clean on `pinot-core`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
