Jackie-Jiang commented on code in PR #10028:
URL: https://github.com/apache/pinot/pull/10028#discussion_r1056776854
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java:
##########
@@ -272,6 +272,7 @@ public static Map<String, String>
getInstanceStateMap(Collection<String> instanc
return instanceStateMap;
}
+
Review Comment:
(nit) remove
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java:
##########
@@ -292,6 +293,21 @@ public static Map<String, Integer>
getNumSegmentsToBeMovedPerInstance(Map<String
return numSegmentsToBeMovedPerInstance;
}
+ public static Set<String> getSegmentsToMove(Map<String, Map<String, String>>
oldAssignment,
+ Map<String, Map<String, String>> newAssignment) {
+ Set<String> result = new HashSet<>();
+ for (Map.Entry<String, Map<String, String>> entry :
newAssignment.entrySet()) {
+ String segmentName = entry.getKey();
+ Set<String> newInstancesAssigned = entry.getValue().keySet();
+ Set<String> oldInstancesAssigned =
oldAssignment.get(segmentName).keySet();
+ if (!newInstancesAssigned.containsAll(oldInstancesAssigned) ||
!oldInstancesAssigned.containsAll(
+ newInstancesAssigned)) {
Review Comment:
We can simplify it and directly compare the assignment
```suggestion
if (!entry.getValue().equals(oldAssignment.get(segmentName)) {
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -304,11 +304,18 @@ public RebalanceResult rebalance(TableConfig tableConfig,
Configuration rebalanc
LOGGER.info("Rebalancing table: {} with minAvailableReplicas: {},
enableStrictReplicaGroup: {}, bestEfforts: {}",
tableNameWithType, minAvailableReplicas, enableStrictReplicaGroup,
bestEfforts);
int expectedVersion = currentIdealState.getRecord().getVersion();
- while (true) {
- // Wait for ExternalView to converge before updating the next IdealState
- IdealState idealState;
+
+ do {
+ Map<String, Map<String, String>> nextAssignment =
+ getNextAssignment(currentAssignment, targetAssignment,
minAvailableReplicas, enableStrictReplicaGroup);
+ LOGGER.info("Got the next assignment for table: {} with number of
segments to be moved to each instance: {}",
+ tableNameWithType,
+
SegmentAssignmentUtils.getNumSegmentsToBeMovedPerInstance(currentAssignment,
nextAssignment));
+
+ Set<String> segmentsToMove =
SegmentAssignmentUtils.getSegmentsToMove(currentAssignment, nextAssignment);
Review Comment:
`segmentsToMove` can be calculated with current and target assignment. We
can keep the current logic of calculating next assignment.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
