Jackie-Jiang commented on code in PR #17707:
URL: https://github.com/apache/pinot/pull/17707#discussion_r2819798699
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -694,8 +692,28 @@ 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) {
Review Comment:
(MAJOR) This logic doesn't handle the table paused scenario, where there
will be a dangling consuming segment ZK metadata. Please add a test to guard
this
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -655,10 +656,7 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
String committingSegmentName =
committingSegmentDescriptor.getSegmentName();
TableConfig tableConfig = getTableConfig(realtimeTableName);
InstancePartitions instancePartitions =
getConsumingInstancePartitions(tableConfig);
- IdealState idealState = getIdealState(realtimeTableName);
- Preconditions.checkState(
-
idealState.getInstanceStateMap(committingSegmentName).containsValue(SegmentStateModel.CONSUMING),
- "Failed to find instance in CONSUMING state in IdealState for segment:
%s", committingSegmentName);
+ IdealState idealState;
Review Comment:
(minor) Move the declaration below (to line 695)
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1428,6 +1465,10 @@ IdealState updateIdealStateOnSegmentCompletion(String
realtimeTableName, String
throw new HelixHelper.PermanentUpdaterException(
"Exceeded max segment completion time for segment " +
committingSegmentName);
}
+ Map<String, String> committingSegmentInstanceStateMap =
idealState.getInstanceStateMap(committingSegmentName);
+ Preconditions.checkState(committingSegmentInstanceStateMap != null &&
committingSegmentInstanceStateMap
+ .containsValue(SegmentStateModel.CONSUMING),
+ "Failed to find instance in CONSUMING state in IdealState for
segment: %s", committingSegmentName);
Review Comment:
(minor) Move this check into `updateInstanceStatesForNewConsumingSegment()`
--
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]
