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 understand it is intersecting the same list, but I can at
least give it a good reason.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
