[GitHub] [helix] jiajunwang commented on a change in pull request #986: Adjust the auto rebalancer state assignment logic to reduce top state transition.

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #986:
URL: https://github.com/apache/helix/pull/986#discussion_r418780668



##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/AbstractRebalancer.java
##########
@@ -305,45 +305,60 @@ public static int getStateCount(String state, StateModelDefinition stateModelDef
     Set<String> liveAndEnabled = new HashSet<>(liveInstances);
     liveAndEnabled.removeAll(disabledInstancesForPartition);
 
-    for (String state : statesPriorityList) {
-      // Use the the specially ordered preferenceList for choosing instance for top state.
-      if (state.equals(statesPriorityList.get(0))) {
-        List<String> preferenceListForTopState = new ArrayList<>(preferenceList);
-        Collections.sort(preferenceListForTopState,
-            new TopStatePreferenceListComparator(currentStateMap, stateModelDef));
-        preferenceList = preferenceListForTopState;
-      }
+    // Sort the instances based on replicas' state priority in the current state
+    List<String> sortedPreferenceList = new ArrayList<>(preferenceList);
+    sortedPreferenceList.sort(new StatePriorityComparator(currentStateMap, stateModelDef));
 
+    // Assign the state to the instances that appear in the preference list.
+    for (String state : statesPriorityList) {
       int stateCount =
           getStateCount(state, stateModelDef, liveAndEnabled.size(), preferenceList.size());
       for (String instance : preferenceList) {
         if (stateCount <= 0) {
-          break;
+          break; // continue assigning for the next state
+        }
+        if (assigned.contains(instance) || !liveAndEnabled.contains(instance)) {
+          continue; // continue checking for the next available instance
         }
-        if (!assigned.contains(instance) && liveAndEnabled.contains(instance)) {
-          if (HelixDefinedState.ERROR.toString().equals(currentStateMap.get(instance))) {
-            bestPossibleStateMap.put(instance, HelixDefinedState.ERROR.toString());
-          } else {
-            bestPossibleStateMap.put(instance, state);
-            stateCount--;
+        String proposedInstance = instance;
+        // Additional check and alternate the assignment for reducing top state handoff.
+        if (state.equals(stateModelDef.getTopState()) && !stateModelDef.getSecondTopStates()

Review comment:
       I thought about it. But even I move this logic to the sorting logic, the sorting itself will be comparing one list with 2 logics. This interesting trick is just moved to another place. I don't have a way to eliminate it for now.
   Then why I want to put it here instead of the sorting logic? Because the sorting logic is away from this business logic block. That would be even harder for me to reason even with comments.Put it here, I 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org