zpinto commented on code in PR #2772:
URL: https://github.com/apache/helix/pull/2772#discussion_r1548991400
##########
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java:
##########
@@ -161,49 +162,47 @@ private void
addSwapInInstancesToBestPossibleState(Map<String, Resource> resourc
return;
}
- // If the corresponding swap-in instance is not enabled,
assign replicas with
- // initial state.
- if (!enabledSwapInInstances.contains(
- swapOutToSwapInInstancePairs.get(swapOutInstance))) {
-
stateMap.put(swapOutToSwapInInstancePairs.get(swapOutInstance),
- stateModelDef.getInitialState());
- return;
- }
-
- // If the swap-in node is live and enabled, do assignment with
the following logic:
- // 1. If the swap-out instance's replica is a secondTopState,
set the swap-in instance's replica
- // to the same secondTopState.
- // 2. If the swap-out instance's replica is any other state
and is in the preferenceList,
- // set the swap-in instance's replica to the topState if the
StateModel allows another to be added.
- // If not, set the swap-in instance's replica to the
secondTopState.
- // We can make this assumption because if there is assignment
to the swapOutInstance, it must be either
- // a topState or a secondTopState.
- if (stateMap.containsKey(swapOutInstance) &&
stateModelDef.getSecondTopStates()
- .contains(stateMap.get(swapOutInstance))) {
- // If the swap-out instance's replica is a secondTopState,
set the swap-in instance's replica
- // to the same secondTopState.
-
stateMap.put(swapOutToSwapInInstancePairs.get(swapOutInstance),
- stateMap.get(swapOutInstance));
- } else {
- // If the swap-out instance's replica is any other state in
the stateMap or not present in the
- // stateMap, set the swap-in instance's replica to the
topState if the StateModel allows another
- // to be added. If not, set the swap-in to the
secondTopState.
- String topStateCount =
-
stateModelDef.getNumInstancesPerState(stateModelDef.getTopState());
- if (topStateCount.equals(
-
StateModelDefinition.STATE_REPLICA_COUNT_ALL_CANDIDATE_NODES)
- || topStateCount.equals(
- StateModelDefinition.STATE_REPLICA_COUNT_ALL_REPLICAS)) {
- // If the StateModel allows for another replica with the
topState to be added,
- // set the swap-in instance's replica to the topState.
-
stateMap.put(swapOutToSwapInInstancePairs.get(swapOutInstance),
- stateModelDef.getTopState());
+ // If the swap-in node is live, do assignment with the
following logic:
+ // 1. If the swap-out instance's replica is in the stateMap:
+ // - if the swap-out instance's replica is a topState, set the
swap-in instance's replica to the topState.
+ // if another is allowed to be added, otherwise set the
swap-in instance's replica to a secondTopState.
+ // - if the swap-out instance's replica is not a topState or
ERROR, set the swap-in instance's replica to the same state.
+ // - if the swap-out instance's replica is ERROR, set the
swap-in instance's replica to the initialState.
+ // 2. If the swap-out instance's replica is not in the
stateMap, set the swap-in instance's replica to the initialState.
+ // This happens when the swap-out node is offline.
+ if (stateMap.containsKey(swapOutInstance)) {
+ if
(stateMap.get(swapOutInstance).equals(stateModelDef.getTopState())
+ ||
stateMap.get(swapOutInstance).equals(HelixDefinedState.ERROR.name())) {
+ // If the swap-out instance's replica is a topState, set
the swap-in instance's replica
+ // to the topState if the StateModel allows another to be
added. If not, set the swap-in
+ // to the secondTopState.
+ String topStateCount =
+
stateModelDef.getNumInstancesPerState(stateModelDef.getTopState());
+ if (topStateCount.equals(
+
StateModelDefinition.STATE_REPLICA_COUNT_ALL_CANDIDATE_NODES)
+ || topStateCount.equals(
+
StateModelDefinition.STATE_REPLICA_COUNT_ALL_REPLICAS)) {
+ // If the StateModel allows for another replica with the
topState to be added,
+ // set the swap-in instance's replica to the topState.
+
stateMap.put(swapOutToSwapInInstancePairs.get(swapOutInstance),
+ stateModelDef.getTopState());
+ } else {
+ // If StateModel does not allow another topState replica
to be
+ // added, set the swap-in instance's replica to the
secondTopState.
+
stateMap.put(swapOutToSwapInInstancePairs.get(swapOutInstance),
+
stateModelDef.getSecondTopStates().iterator().next());
+ }
} else {
- // If StateModel does not allow another topState replica
to be
- // added, set the swap-in instance's replica to the
secondTopState.
+ // If the swap-out instance's replica is not a topState or
ERROR, set the swap-in instance's replica
+ // to the same state
stateMap.put(swapOutToSwapInInstancePairs.get(swapOutInstance),
- stateModelDef.getSecondTopStates().iterator().next());
+ stateMap.get(swapOutInstance));
}
+ } else {
+ // If the swap-out instance's replica is not in the
stateMap, set the swap-in instance's replica
+ // to the initialState. This happens when the swap-out node
is offline.
+
stateMap.put(swapOutToSwapInInstancePairs.get(swapOutInstance),
+ stateModelDef.getInitialState());
}
});
});
Review Comment:
I have separated assignment and and state selection into two steps to make
it more debuggable.
--
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: reviews-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
