suvodeep-pyne commented on code in PR #19995:
URL: https://github.com/apache/pinot/pull/19995#discussion_r4343161840
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -718,6 +722,15 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
removeSegmentZKMetadataBestEffort(realtimeTableName,
newConsumingSegmentName);
newConsumingSegmentName = null;
}
+ } else if (newSegmentOfEarlierAttempt != null
+ &&
!instanceStatesMapAfterStep3.containsKey(newSegmentOfEarlierAttempt)) {
+ // The committing segment is ONLINE now, so no attempt of the commit can
add the new segment of the earlier
+ // attempt anymore. Remove it so that RealtimeSegmentValidationManager
does not add it either.
+ LOGGER.info(
+ "Cleaning up segment ZK metadata for new segment {} of table {}
created by an earlier attempt of the commit "
+ + "because it is not used. This can happen when table/topic
consumption is paused, or the partition is "
+ + "no longer in the stream.", newSegmentOfEarlierAttempt,
realtimeTableName);
+ removeSegmentZKMetadataBestEffort(realtimeTableName,
newSegmentOfEarlierAttempt);
Review Comment:
Thanks, agreed, the snapshot-based delete was not safe. Fixed in f3c514c99f:
the cleanup now goes through
PinotHelixResourceManager#removeInProgressSegmentsZKMetadataNotInIdealState. It
removes only IN_PROGRESS segments that are not in the IdealState, in one ZK
multi() with version-checked deletes of their ZK metadata and a version-checked
write of the unchanged IdealState:
- if validation read the IdealState before the removal, its write fails, and
its retry reads the segment ZK metadata again and no longer finds the segment;
- if validation added the segment first, the IdealState version changed, so
the removal fails and its retry keeps the segment.
Tests:
- testCommitStartRetryKeepsNewSegmentOfEarlierAttemptAddedByValidation: the
retry blocks before the removal while the table is resumed and validation adds
the kept segment; its ZK metadata is kept.
- Against a real ZooKeeper in PinotHelixResourceManagerStatelessTest: the
segment added to the IdealState right after the removal read it is kept; its ZK
metadata overwritten right after the read is kept; an IdealState write based on
the IdealState read before the removal fails.
Removing either version check fails the matching test. The previous cleanup
of a new segment not added to the IdealState (pause between steps 2 and 3) uses
the same fenced removal.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -673,9 +673,13 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
// Step-2: Create new segment metadata if needed
long startTimeNs2 = System.nanoTime();
+ // Another attempt of the same commit start might have created the new
segment already. Find it before deciding
+ // whether the partition still needs a new segment, so that it can be
removed when it is not used.
+ String newSegmentOfEarlierAttempt =
+ resumed ? findNewSegmentOfEarlierAttempt(realtimeTableName, new
LLCSegmentName(committingSegmentName)) : null;
Review Comment:
Fixed in f3c514c99f: the lookup now returns every new segment of earlier
attempts in the window, not just the first. The commit reuses the oldest one
when the partition still needs a new segment, and removes all the unused ones
through the fenced removal.
testCommitStartRetryWhileTablePausedRemovesNewSegmentsOfEarlierAttempts covers
2 kept segments from overlapping attempts; it fails if the lookup stops at the
first one.
--
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]
