shuturmurgh commented on code in PR #20111:
URL: https://github.com/apache/pinot/pull/20111#discussion_r4502447264
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -4682,6 +4682,18 @@ public String startReplaceSegments(String
tableNameWithType, List<String> segmen
// 2. Proactively delete the oldest data snapshot to make sure
that we only keep at most 2 data snapshots
// at any time in case of REFRESH use case.
if (forceCleanup) {
+ // A 'COMPLETED' entry has already replaced its 'segmentsFrom',
so replacing them again would leave two
+ // 'COMPLETED' entries claiming the same source and serve both
outputs. The 'segmentsFrom' existence
+ // check above runs outside the lineage lock, so a concurrent
'endReplaceSegments' can complete an entry
+ // after that check passed. Re-validate against the lineage read
under the lock; 'forceCleanup' only
+ // un-blocks incomplete entries, it must not allow replacing a
replaced segment.
+ if (lineageEntry.getState() == LineageEntryState.COMPLETED &&
!Collections.disjoint(segmentsFrom,
Review Comment:
Good catch, agreed. The pre-lock check misses the empty `segmentsFrom` case
and never compares `segmentsTo` against `COMPLETED` entries, so B could be
accepted over A's committed output.
Fixed in the latest commit:
- Moved both existence checks into a helper,
`validateSegmentsForStartReplace`, and now also run it **inside the retry
lambda**. It checks the ideal state filtered by the segment lineage read in
that attempt, and the versioned lineage write guards that read. This covers
both `forceCleanup` modes, including the empty-`segmentsFrom` initial push.
- Removed the `COMPLETED`-only `segmentsFrom` block, since the in-lock check
makes it redundant.
I kept the same check **before** the lock as well. Exceptions thrown inside
the retry come back wrapped as `Failed to update the segment lineage during
startReplaceSegments...`. The early check keeps the common, non-racing case
failing fast with the specific `IllegalStateException` message that existing
tests and REST callers see today. The cost is one extra ideal-state read per
attempt, which `endReplaceSegments` already does.
Added
`testStartReplaceSegmentsForceCleanupRejectsConcurrentlyCommittedSegmentsTo`: A
pushes `[] -> [a0]`, and B starts `[] -> [a0, b1]` while A completes. B is now
rejected with `Segment: a0 from 'segmentsTo' exists`, and only A's entry
remains. On the previous code the test fails because B is accepted and creates
a lineage entry.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -4682,6 +4682,18 @@ public String startReplaceSegments(String
tableNameWithType, List<String> segmen
// 2. Proactively delete the oldest data snapshot to make sure
that we only keep at most 2 data snapshots
// at any time in case of REFRESH use case.
if (forceCleanup) {
+ // A 'COMPLETED' entry has already replaced its 'segmentsFrom',
so replacing them again would leave two
+ // 'COMPLETED' entries claiming the same source and serve both
outputs. The 'segmentsFrom' existence
+ // check above runs outside the lineage lock, so a concurrent
'endReplaceSegments' can complete an entry
+ // after that check passed. Re-validate against the lineage read
under the lock; 'forceCleanup' only
+ // un-blocks incomplete entries, it must not allow replacing a
replaced segment.
+ if (lineageEntry.getState() == LineageEntryState.COMPLETED &&
!Collections.disjoint(segmentsFrom,
Review Comment:
Done in the latest commit:
- `segmentsFrom` and `segmentsTo` are now `HashSet`s built once, outside the
retry lambda, and the `IN_PROGRESS` overlap check uses them. The set is the
first argument to `Collections.disjoint`, so each entry costs O(size of that
entry).
- The message with both full `segmentsFrom` lists went away with the
`COMPLETED` block. The shared check fails on the first conflicting segment and
names only that segment.
--
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]
