xiangfu0 commented on code in PR #19054:
URL: https://github.com/apache/pinot/pull/19054#discussion_r3635509479
##########
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
+ /// target derived from it (via `rebalanceTable` / `getNextAssignment`),
which covers every current segment, and
+ /// every segment has at least one replica.
+ private static boolean isMovingConsumingSegment(Map<String, String>
currentInstanceStateMap,
+ Map<String, String> targetInstanceStateMap) {
+ return
targetInstanceStateMap.values().iterator().next().equals(SegmentStateModel.CONSUMING)
Review Comment:
[MAJOR] Do not infer the segment state from the first replica.
`getNextSingleSegmentAssignment()` can retain an old replica's `OFFLINE` state
while adding target replicas as `CONSUMING`, producing a valid intermediate map
such as `{server1=OFFLINE, server3=CONSUMING}`. Because this map is a
`TreeMap`, this line can read `OFFLINE` first and miss a consuming segment that
is about to move. With `forceCommit=true`, the rebalance then skips the force
commit and moves that segment, risking re-consumption or peer-download failure.
Please retain the previous `no ONLINE && any CONSUMING` classification
(including the null guard), or scan all values, and restore the mixed-state
regression case removed in this PR.
--
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]
