Jackie-Jiang commented on code in PR #19995:
URL: https://github.com/apache/pinot/pull/19995#discussion_r4342857207
##########
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:
Remaining pause-cleanup gap: more than one successor can be retained
The probe returns only the first matching name, but overlapping controllers
can leave multiple successors with the same sequence number and different
minute timestamps. For example, A marks the predecessor COMMITTING and stalls
before step 2; new leader B resumes, finds no successor, creates N1, and fails
its IdealState update. A then continues in a later minute with its original
resumed=false, creates N2, and also fails. Both failure handlers retain their
metadata while the predecessor is still CONSUMING.
A subsequent paused retry finds and removes only N1. N2 remains, so
validation can still adopt it after the timeout and ignore the offset requested
when the table resumes. This is a remaining variant of the original leak rather
than a new regression in this PR.
Please add a two-retained-successor regression test and safely handle all
unused candidates, preserving the protection against removing metadata for a
segment adopted concurrently.
##########
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:
[P1] Fence cleanup against validation adopting the retained segment
This deletion relies on the IdealState snapshot returned by step 3. A paused
retry can mark the predecessor ONLINE and then stall before this removal. If
the table resumes and the retained successor passes its completion timeout,
validation can adopt that same successor as CONSUMING: its
missing-previous-CONSUMING branch only logs an error, then calls
updateInstanceStatesForNewConsumingSegment with a null predecessor. The stalled
retry subsequently deletes the live successor's metadata using its stale
snapshot.
The predecessor being ONLINE fences other commit attempts, but not this
validation path. This interleaving uses the original retained segment across
minutes; it does not require same-minute recreation. A retry near the
completion deadline can make the required delay short.
Please coordinate deletion with validation's ability to adopt the segment
(an ordinary IdealState re-read alone still leaves a race). Add a deterministic
regression test that blocks cleanup before removal, lets resume/validation
install the successor, and verifies that cleanup cannot remove its metadata.
--
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]
