tarun11Mavani commented on code in PR #20111:
URL: https://github.com/apache/pinot/pull/20111#discussion_r4492356012
##########
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:
This only handles COMPLETED entries whose `segmentsFrom` overlaps the
request. The pre-lock validation is still stale in other cases:
- With empty `segmentsFrom` (initial push), `Collections.disjoint([], x)` is
always true, so the check never fires.
- Force mode never compares `segmentsTo` against COMPLETED entries. The
strict branch does, at line 4763.
If A completes between B's pre-lock check and the lock, B can be accepted
over A's committed `segmentsTo`. Reverting B would then delete that data.
Suggest moving the validation at 4620-4631 into the retry lambda. That
covers both modes and makes this block redundant. Otherwise, hoist this check
above `if (forceCleanup)` and add the `segmentsTo` comparison.
##########
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:
`Collections.disjoint` on two `List`s is O(n·m), and it now runs against
every COMPLETED entry under the lineage lock, inside the retry loop. The strict
branch (line 4753) already uses a `HashSet` for this.
Suggest building a `Set<String>` from `segmentsFrom` once, outside the
lambda, and using it here and in the IN_PROGRESS check below.
Also, the exception message embeds the full `segmentsFrom` of both the
request and the entry. Reporting only the intersection would keep it bounded.
--
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]
