You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2021/02/23 03:51:01 UTC
[GitHub] [helix] dasahcc commented on a change in pull request #1628: Per Replica Throttle -- 2nd: messages classification and basic throttle application
dasahcc commented on a change in pull request #1628:
URL: https://github.com/apache/helix/pull/1628#discussion_r580751355
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/PerReplicaThrottleStage.java
##########
@@ -220,6 +271,274 @@ private MessageOutput throttlePerReplicaMessages(IdealState idealState,
return output;
}
+ /*
+ * Let us use an example to illustrate the state count accumulation idea:
+ * Currentstate N1(O), N2(S), N3(O), message N3(O->S) should be classified as recovery.
+ * Assuming without the propagateCounts logic,
+ * Here, the expectedStateCounts is {M->1, S->1, O->0}, given MIN_ACTIVE=2.
+ * The currentStateCounts is {M->0, S->1, O->0}.
+ * At the time, message N3(O->S) is tested for load/recovery, the logic would compare
+ * currentStateCounts[S] with expectedStateCounts[S]. If less than expected, it is deemed as
+ * recovery; otherwise load.
+ * Then the message would be incorrectly classified as load.
+ *
+ * With propogationTopDown, we have expectedStateCounts as {M->1, S->2 O->3}.
+ * currentStateCountCounts as {M->0, S->1, O->1}. Thus the message is going to be classified
+ * as recovery correctly.
+ *
+ * The gist is that:
+ * When determining a message as LOAD or RECOVERY, we look at the toState of this message.
+ * If the accumulated current count of toState meet the required accumulated expected count
+ * of the toState, we will treat it as Load, otherwise, it is Recovery.
+ *
+ * Note, there can be customerized model that having more than one route to a top state. For
+ * example S1, S2, S3 are three levels of states with S1 as top state with lowest priority.
+ * It is possible that S2 can transits upward to S1 while S3 can also transits upward to S1.
+ * Thus, we consider S1 meet count requirement of both S2 and S3. In propagation time, we will
+ * add state count of S1 to both S2 and S3.
+ */
+ private void propagateCountsTopDown(StateModelDefinition stateModelDef,
+ Map<String, Integer> stateCountMap) {
+ List<String> stateList = stateModelDef.getStatesPriorityList();
+ if (stateList == null || stateList.size() <= 0) {
+ return;
+ }
+
+ Map<String, Integer> statePriorityMap = new HashMap<>();
+
+ // calculate rank of each state. Next use the rank to compare if a transition is upward or not
+ int rank = 0;
+ for (String state : stateList) {
+ statePriorityMap.put(state, Integer.valueOf(rank));
+ rank++;
Review comment:
I believe state model should have the function to get priority of states. Even if it does not have it, this computation logic should be there.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/PerReplicaThrottleStage.java
##########
@@ -220,6 +271,274 @@ private MessageOutput throttlePerReplicaMessages(IdealState idealState,
return output;
}
+ /*
+ * Let us use an example to illustrate the state count accumulation idea:
+ * Currentstate N1(O), N2(S), N3(O), message N3(O->S) should be classified as recovery.
+ * Assuming without the propagateCounts logic,
+ * Here, the expectedStateCounts is {M->1, S->1, O->0}, given MIN_ACTIVE=2.
+ * The currentStateCounts is {M->0, S->1, O->0}.
+ * At the time, message N3(O->S) is tested for load/recovery, the logic would compare
+ * currentStateCounts[S] with expectedStateCounts[S]. If less than expected, it is deemed as
+ * recovery; otherwise load.
+ * Then the message would be incorrectly classified as load.
+ *
+ * With propogationTopDown, we have expectedStateCounts as {M->1, S->2 O->3}.
+ * currentStateCountCounts as {M->0, S->1, O->1}. Thus the message is going to be classified
+ * as recovery correctly.
+ *
+ * The gist is that:
+ * When determining a message as LOAD or RECOVERY, we look at the toState of this message.
+ * If the accumulated current count of toState meet the required accumulated expected count
+ * of the toState, we will treat it as Load, otherwise, it is Recovery.
+ *
+ * Note, there can be customerized model that having more than one route to a top state. For
+ * example S1, S2, S3 are three levels of states with S1 as top state with lowest priority.
+ * It is possible that S2 can transits upward to S1 while S3 can also transits upward to S1.
+ * Thus, we consider S1 meet count requirement of both S2 and S3. In propagation time, we will
+ * add state count of S1 to both S2 and S3.
+ */
+ private void propagateCountsTopDown(StateModelDefinition stateModelDef,
+ Map<String, Integer> stateCountMap) {
+ List<String> stateList = stateModelDef.getStatesPriorityList();
+ if (stateList == null || stateList.size() <= 0) {
+ return;
+ }
+
+ Map<String, Integer> statePriorityMap = new HashMap<>();
+
+ // calculate rank of each state. Next use the rank to compare if a transition is upward or not
+ int rank = 0;
+ for (String state : stateList) {
+ statePriorityMap.put(state, Integer.valueOf(rank));
+ rank++;
+ }
+
+ // given a key state, find the set of states that can transit to this key state upwards.
+ Map<String, Set<String>> fromStatesMap = new HashMap<>();
+ for (String transition : stateModelDef.getStateTransitionPriorityList()) {
+ // Note, we assume stateModelDef is properly constructed.
+ String[] fromStateAndToState = transition.split("-");
+ String fromState = fromStateAndToState[0];
+ String toState = fromStateAndToState[1];
+ Integer fromStatePriority = statePriorityMap.get(fromState);
+ Integer toStatePriority = statePriorityMap.get(toState);
+ if (fromStatePriority.compareTo(toStatePriority) <= 0) {
+ // skip downward transitition
+ continue;
+ }
+ fromStatesMap.putIfAbsent(toState, new HashSet<>());
+ fromStatesMap.get(toState).add(fromState);
+ }
+
+ // propagation by adding state counts of current state to all lower priority state that can
+ // transit to this current state
+ int index = 0;
+ while (true) {
Review comment:
It is not safe to use while (true). The only condition is index < stateList.size(), then let's put it here.
Or we can just use for loop to lopp state list.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/PerReplicaThrottleStage.java
##########
@@ -220,6 +271,274 @@ private MessageOutput throttlePerReplicaMessages(IdealState idealState,
return output;
}
+ /*
+ * Let us use an example to illustrate the state count accumulation idea:
+ * Currentstate N1(O), N2(S), N3(O), message N3(O->S) should be classified as recovery.
+ * Assuming without the propagateCounts logic,
+ * Here, the expectedStateCounts is {M->1, S->1, O->0}, given MIN_ACTIVE=2.
+ * The currentStateCounts is {M->0, S->1, O->0}.
+ * At the time, message N3(O->S) is tested for load/recovery, the logic would compare
+ * currentStateCounts[S] with expectedStateCounts[S]. If less than expected, it is deemed as
+ * recovery; otherwise load.
+ * Then the message would be incorrectly classified as load.
+ *
+ * With propogationTopDown, we have expectedStateCounts as {M->1, S->2 O->3}.
+ * currentStateCountCounts as {M->0, S->1, O->1}. Thus the message is going to be classified
+ * as recovery correctly.
+ *
+ * The gist is that:
+ * When determining a message as LOAD or RECOVERY, we look at the toState of this message.
+ * If the accumulated current count of toState meet the required accumulated expected count
+ * of the toState, we will treat it as Load, otherwise, it is Recovery.
+ *
+ * Note, there can be customerized model that having more than one route to a top state. For
+ * example S1, S2, S3 are three levels of states with S1 as top state with lowest priority.
+ * It is possible that S2 can transits upward to S1 while S3 can also transits upward to S1.
+ * Thus, we consider S1 meet count requirement of both S2 and S3. In propagation time, we will
+ * add state count of S1 to both S2 and S3.
+ */
+ private void propagateCountsTopDown(StateModelDefinition stateModelDef,
+ Map<String, Integer> stateCountMap) {
+ List<String> stateList = stateModelDef.getStatesPriorityList();
+ if (stateList == null || stateList.size() <= 0) {
+ return;
+ }
+
+ Map<String, Integer> statePriorityMap = new HashMap<>();
+
+ // calculate rank of each state. Next use the rank to compare if a transition is upward or not
+ int rank = 0;
+ for (String state : stateList) {
+ statePriorityMap.put(state, Integer.valueOf(rank));
Review comment:
So the rank is int, why we need Integer.valueOf(rank)?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org