You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by jx...@apache.org on 2018/10/30 00:54:09 UTC
helix git commit: [HELIX-770] HELIX: Fix a possible NPE in
loadBalance in IntermediateStateCalcStage
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 <hu...@linkedin.com>
Authored: Mon Oct 29 16:50:41 2018 -0700
Committer: Hunter Lee <hu...@linkedin.com>
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");
}
}