xiangfu0 commented on code in PR #20111:
URL: https://github.com/apache/pinot/pull/20111#discussion_r4503034592
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -4617,18 +4617,10 @@ public String startReplaceSegments(String
tableNameWithType, List<String> segmen
// Create a segment lineage entry id
String segmentLineageEntryId =
SegmentLineageUtils.generateLineageEntryId();
- // Check that all the segments from 'segmentsFrom' exist in the table
- Set<String> segmentsForTable = new
HashSet<>(getSegmentsFor(tableNameWithType, true));
- for (String segment : segmentsFrom) {
- Preconditions.checkState(segmentsForTable.contains(segment),
- "Segment: %s from 'segmentsFrom' does not exist in table: %s",
segment, tableNameWithType);
- }
-
- // Check that all the segments from 'segmentTo' does not exist in the table
- for (String segment : segmentsTo) {
- Preconditions.checkState(!segmentsForTable.contains(segment), "Segment:
%s from 'segmentsTo' exists in table: %s",
- segment, tableNameWithType);
- }
+ validateSegmentsForStartReplace(tableNameWithType, segmentsFrom,
segmentsTo,
+ new HashSet<>(getSegmentsFor(tableNameWithType, true)));
+ Set<String> segmentsFromSet = new HashSet<>(segmentsFrom);
+ Set<String> segmentsToSet = new HashSet<>(segmentsTo);
List<String> segmentsToCleanUp = new ArrayList<>();
Review Comment:
Not blocking, and older than this PR: `segmentsToCleanUp` carries over
between retry attempts. Here is a narrow case the new check doesn't catch.
Attempt 1 reverts `IN_PROGRESS` entry X because only `segmentsTo` overlaps,
then loses the version race to X's `endReplaceSegments` on another controller.
X ends with a subset of its `segmentsTo` that excludes the overlapping segment.
Attempt 2 then succeeds, and X's committed output gets deleted. Calling
`segmentsToCleanUp.clear()` at the start of each attempt would close this,
since every attempt rebuilds the list from its own lineage read. (Found by
reading the code; not reproduced.)
--
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]
