Copilot commented on code in PR #17707:
URL: https://github.com/apache/pinot/pull/17707#discussion_r2811557197
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -694,8 +692,24 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
// the idealstate update fails due to contention. We serialize the updates
to the idealstate
// to reduce this contention. We may still contend with RetentionManager,
or other updates
// to idealstate from other controllers, but then we have the retry
mechanism to get around that.
- idealState =
- updateIdealStateForSegments(tableConfig, committingSegmentName,
newConsumingSegmentName, instancePartitions);
+ try {
+ idealState =
+ updateIdealStateForSegments(tableConfig, committingSegmentName,
newConsumingSegmentName, instancePartitions);
+ } catch (RuntimeException e) {
+ if (newConsumingSegmentName != null) {
+ removeSegmentZKMetadataBestEffort(realtimeTableName,
newConsumingSegmentName);
+ }
+ throw e;
+ }
+
+ boolean newConsumingSegmentInIdealState = false;
+ if (newConsumingSegmentName != null) {
+ newConsumingSegmentInIdealState =
idealState.getRecord().getMapFields().containsKey(newConsumingSegmentName);
+ if (!newConsumingSegmentInIdealState) {
Review Comment:
The cleanup logic checks if newConsumingSegmentName is present in
IdealState's mapFields, but doesn't distinguish between "Step-3 didn't add it
because table/topic is paused" vs "Step-3 didn't add it for other reasons".
When table/topic is paused, updateIdealStateOnSegmentCompletion at line 1469
passes null for newSegmentName, causing
updateInstanceStatesForNewConsumingSegment to skip adding the segment. This is
correct behavior, but consider adding a log message in the cleanup block (lines
707-711) to clarify why the segment was removed, especially noting if it's due
to pause state. This would help operators understand why segment metadata was
created and then immediately cleaned up.
```suggestion
if (!newConsumingSegmentInIdealState) {
LOGGER.info(
"Cleaning up segment ZK metadata for new consuming segment {} of
table {} because it was not added to "
+ "IdealState. This can happen, for example, when the table
or stream is paused.",
newConsumingSegmentName, realtimeTableName);
```
--
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]
