deepthi912 opened a new pull request, #19512: URL: https://github.com/apache/pinot/pull/19512
## Problem `BasePartitionUpsertMetadataManager.doAddSegment`, `doPreloadSegment`, and `doReplaceSegment` currently advance `_largestSeenComparisonValue` **before** the segment's rows are inserted into `_primaryKeyToRecordLocationMap`. A concurrent `removeExpiredPrimaryKeys` sweep on another thread reads the newly-advanced watermark and can expire pointers for primary keys the in-flight add is about to insert, producing duplicate first-row inserts on those keys. Race timeline (`doAddSegment` for segment with `max=1000`, `metadataTTL=300`, previous watermark `500`): 1. T1 bumps `_largestSeenComparisonValue` from 500 → 1000. 2. T2 (`removeExpiredPrimaryKeys`) reads watermark = 1000, expires all PKs with comparison value `< 700`. 3. T1 inserts segment's rows; any key T2 just expired that this segment carries lands with no prior pointer, becoming a first-row insert → duplicate visible row. ## Fix Move the watermark bump to **after** `addSegment`/`doPreloadSegment`/`replaceSegment` returns, and wrap it in a `finally` block so a failed add still records the max we've seen. The out-of-TTL skip path bumps the watermark before returning as before, since it inserts no rows and cannot race with the sweep. Per-partition state transitions are serialized by Helix, so concurrent `doAddSegment` invocations on the same partition are not possible; the only concurrency here is with the sweep thread. ## Reproduction Reproduced with [`startreedata/theseus-upsert-verifier`](https://github.com/startreedata/theseus-upsert-verifier), a Hypothesis-driven model of Pinot upsert coordination. The `H015`-class duplicate visible row is triggered by restart → preload → offload → metadataTTL sweep interleavings and is confirmed closed by this change: ``` uv run python simulate_handoff.py --preset restart --restarts 2 \ --fix rocksdb-reuse --tasks --profile mttl-deletes --examples 5000 ``` Without this fix the verifier finds a P1v violation (`visible rows differ from the fold plus ghosts`) in ~42 s. ## Testing - Existing 56 upsert unit tests in `pinot-segment-local` pass unchanged (`ConcurrentMapPartitionUpsertMetadataManagerTest`, `BasePartitionUpsertMetadataManagerTest`, `ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletesTest`). - `spotless:apply`, `checkstyle:check`, `license:check` all clean on the affected module. An in-process regression test requires a testable seam to pause `addSegment` mid-call so the sweep can be raced against it deterministically; primary regression coverage lives in the theseus verifier above. ## Test plan - [ ] Existing `pinot-segment-local` upsert tests pass - [ ] Manual: verify a table with `metadataTTL` set has no duplicate rows across a restart + preload cycle -- 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]
