jiajunwang commented on a change in pull request #986:
URL: https://github.com/apache/helix/pull/986#discussion_r420989676
##########
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:
Not exactly. Actually, I don't agree this is a TODO work to put
everything in a sorting method. That is just an alternative design, and I don't
think it is cleaner than the current one.
Considering the proposed universal sorting method, it is actually merging 2
orders (preference list order and the current state order). However, there is
no clear way to define the priority score.
Note the logic here is that if the node is not on secondary state or above
but it is attempted to be a master, we should find alternatives according to
the current state order.
This logic cannot be done with only the 2 orders. We have to know the
proposed states assignment. For example, how many top states and where to
assign based on the preference state.
Give this, to merging the sort in one method (instead of the current
design), we will need to:
1. propose a state assignment based on the preference list
2. input this proposed state mapping as one additional input to the sorting
method, and then sort accordingly.
3. calculate another state assignment based on the re-sorted list.
It seems more confusing and slower than the current design.
----------------------------------------------------------------
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]
