Repository: helix Updated Branches: refs/heads/master 7bb55742e -> cf010f904
[HELIX-770] HELIX: Fix a possible NPE in loadBalance in IntermediateStateCalcStage In isLoadBalanceDownwardForAllReplicas() in IntermediateStateCalcStage, statePriorityMap was throwing a NPE because the partition contained a replica in ERROR state, and the map did not have an entry for it. To amend the issue, Venice added the ERROR state in the state model with a priority, and Helix added checks to prevent NPEs. Changelist: 1. Add containsKey checks in isLoadBalanceDownwardForAllReplicas() 2. Make the Controller correctly log all partitions with ERROR state replicas 3. Add HelixDefinedStates in statePriorityList if not already added Project: http://git-wip-us.apache.org/repos/asf/helix/repo Commit: http://git-wip-us.apache.org/repos/asf/helix/commit/cf010f90 Tree: http://git-wip-us.apache.org/repos/asf/helix/tree/cf010f90 Diff: http://git-wip-us.apache.org/repos/asf/helix/diff/cf010f90 Branch: refs/heads/master Commit: cf010f90426003dab7e713945c2c9daa23ffed13 Parents: 7bb5574 Author: Hunter Lee <[email protected]> Authored: Mon Oct 29 16:50:41 2018 -0700 Committer: Hunter Lee <[email protected]> Committed: Mon Oct 29 17:47:51 2018 -0700 ---------------------------------------------------------------------- .../stages/IntermediateStateCalcStage.java | 16 ++++++++++++---- .../apache/helix/model/StateModelDefinition.java | 12 ++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/helix/blob/cf010f90/helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java ---------------------------------------------------------------------- diff --git a/helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java b/helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java index 425f6fa..c156c06 100644 --- a/helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java +++ b/helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java @@ -267,12 +267,13 @@ public class IntermediateStateCalcStage extends AbstractBaseStage { // logic doesn't need to check for more details. boolean isRebalanceNeeded = false; + // Check whether partition has any ERROR state replicas + if (currentStateMap.values().contains(HelixDefinedState.ERROR.name())) { + partitionsWithErrorStateReplica.add(partition); + } + // Number of states required by StateModelDefinition are not satisfied, need recovery if (rebalanceType.equals(RebalanceType.RECOVERY_BALANCE)) { - // Check whether partition is in ERROR state - if (currentStateMap.values().contains(HelixDefinedState.ERROR.name())) { - partitionsWithErrorStateReplica.add(partition); - } // Check if recovery is needed for this partition if (!currentStateMap.equals(bestPossibleMap)) { partitionsNeedRecovery.add(partition); @@ -284,6 +285,7 @@ public class IntermediateStateCalcStage extends AbstractBaseStage { partitionsNeedLoadBalance.add(partition); isRebalanceNeeded = true; } + // Currently at BestPossibleState, no further action necessary if (!isRebalanceNeeded) { Map<String, String> intermediateMap = new HashMap<>(bestPossibleMap); @@ -388,6 +390,12 @@ public class IntermediateStateCalcStage extends AbstractBaseStage { if (bestPossibleState != null) { // Compare priority values and return if an upward transition is found // Note that lower integer value implies higher priority + if (!statePriorityMap.containsKey(currentState) + || !statePriorityMap.containsKey(bestPossibleState)) { + // If the state is not found in statePriorityMap, consider it not strictly downward by + // default because we can't determine whether it is downward + return false; + } if (statePriorityMap.get(currentState) > statePriorityMap.get(bestPossibleState)) { return false; } http://git-wip-us.apache.org/repos/asf/helix/blob/cf010f90/helix-core/src/main/java/org/apache/helix/model/StateModelDefinition.java ---------------------------------------------------------------------- diff --git a/helix-core/src/main/java/org/apache/helix/model/StateModelDefinition.java b/helix-core/src/main/java/org/apache/helix/model/StateModelDefinition.java index 2fbbfcb..ae59522 100644 --- a/helix-core/src/main/java/org/apache/helix/model/StateModelDefinition.java +++ b/helix-core/src/main/java/org/apache/helix/model/StateModelDefinition.java @@ -70,7 +70,7 @@ public class StateModelDefinition extends HelixProperty { private final List<String> _stateTransitionPriorityList; - Map<String, Integer> _statesPriorityMap = new HashMap<String, Integer>(); + private Map<String, Integer> _statesPriorityMap = new HashMap<>(); /** * StateTransition which is used to find the nextState given StartState and @@ -112,9 +112,17 @@ public class StateModelDefinition extends HelixProperty { } } + // add HelixDefinedStates to statesPriorityMap in case it hasn't been added already + for (HelixDefinedState state : HelixDefinedState.values()) { + if (!_statesPriorityMap.containsKey(state.name())) { + // Make it the lowest priority + _statesPriorityMap.put(state.name(), Integer.MAX_VALUE); + } + } + // add transitions for helix-defined states for (HelixDefinedState state : HelixDefinedState.values()) { - if (!_statesPriorityList.contains(state.toString())) { + if (_statesPriorityList == null || !_statesPriorityList.contains(state.toString())) { _statesCountMap.put(state.toString(), "-1"); } }
