Copilot commented on code in PR #19054:
URL: https://github.com/apache/pinot/pull/19054#discussion_r3634946288
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -2233,20 +2248,40 @@ static Set<String>
getMovingConsumingSegments(Map<String, Map<String, String>> c
Map<String, Map<String, String>> targetAssignment) {
Set<String> movingConsumingSegments = new HashSet<>();
for (Map.Entry<String, Map<String, String>> entry :
currentAssignment.entrySet()) {
- String segmentName = entry.getKey();
- Map<String, String> currentInstanceStateMap = entry.getValue();
- Map<String, String> targetInstanceStateMap =
targetAssignment.get(segmentName);
- if (targetInstanceStateMap != null &&
targetInstanceStateMap.values().stream()
- .noneMatch(state -> state.equals(SegmentStateModel.ONLINE)) &&
targetInstanceStateMap.values().stream()
- .anyMatch(state -> state.equals(SegmentStateModel.CONSUMING))) {
- if
(!currentInstanceStateMap.keySet().equals(targetInstanceStateMap.keySet())) {
- movingConsumingSegments.add(segmentName);
- }
+ if (isMovingConsumingSegment(entry.getValue(),
targetAssignment.get(entry.getKey()))) {
+ movingConsumingSegments.add(entry.getKey());
}
}
return movingConsumingSegments;
}
+ /// Returns whether any consuming segment moves between the current and
target assignment, short-circuiting on the
+ /// first such segment. Cheaper than
`!getMovingConsumingSegments(...).isEmpty()` when only existence is needed
+ /// because it does not materialize the full set.
+ @VisibleForTesting
+ static boolean hasMovingConsumingSegments(Map<String, Map<String, String>>
currentAssignment,
+ Map<String, Map<String, String>> targetAssignment) {
+ for (Map.Entry<String, Map<String, String>> entry :
currentAssignment.entrySet()) {
+ if (isMovingConsumingSegment(entry.getValue(),
targetAssignment.get(entry.getKey()))) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /// Returns whether the segment is a consuming segment being moved: its
target state is `CONSUMING` and its assigned
+ /// instances differ between the current and target assignment. A segment's
target instance state map is uniform
+ /// (every replica is `ONLINE`, `CONSUMING`, or `OFFLINE`) because
`RealtimeSegmentAssignment` assigns a single state
+ /// to all instances of a moved segment and keeps `OFFLINE` segments
unchanged, so the segment's state can be read
+ /// from the first entry. The map is non-null and non-empty: callers iterate
the current assignment and look up a
Review Comment:
The new method comment says the target instance-state map is uniform and
that the segment state can be read from the first entry. That’s not true for
intermediate `nextAssignment` maps built by `getNextSingleSegmentAssignment()`
(which can mix states), and it would also become inaccurate if
`isMovingConsumingSegment()` is fixed to scan values.
This issue also appears on line 2279 of the same file.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java:
##########
@@ -2392,67 +2392,99 @@ public void
testRebalanceConsumingSegmentSummaryFailure()
@Test
public void testGetMovingConsumingSegments() {
- // Setup: segment assignments with consuming segments moving
+ // A segment's instance state map is uniform: every replica shares one
state (ONLINE, CONSUMING, or OFFLINE), so
+ // each segment below uses a single state across all its instances.
Review Comment:
The test now states instance-state maps are always uniform, but
`TableRebalancer.getNextSingleSegmentAssignment()` can produce a mixed state
map (e.g., keeping an ONLINE replica to satisfy min-available-replicas while
others are CONSUMING). This test should cover that mixed-state shape to avoid
regressions like relying on `iterator().next()` to infer the segment state.
This issue also appears on line 2430 of the same file.
--
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]
