jiajunwang commented on a change in pull request #1750:
URL: https://github.com/apache/helix/pull/1750#discussion_r638358760
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -513,12 +513,14 @@ private static PipelineRegistry
createDefaultRegistry(String pipelineName) {
// rebalance pipeline
Pipeline rebalancePipeline = new Pipeline(pipelineName);
rebalancePipeline.addStage(new BestPossibleStateCalcStage());
- rebalancePipeline.addStage(new IntermediateStateCalcStage());
// Need to add MaintenanceRecoveryStage here because
MAX_PARTITIONS_PER_INSTANCE check could
// only occur after IntermediateStateCalcStage calculation
rebalancePipeline.addStage(new MaintenanceRecoveryStage());
rebalancePipeline.addStage(new ResourceMessageGenerationPhase());
rebalancePipeline.addStage(new MessageSelectionStage());
+ // Move the IntermediateStateCalcStage after message selection
Review comment:
nit, future code readers won't see this PR directly. Let's change the
description to be "The IntermediateStateCalcStage should be after message
selection."
##########
File path:
helix-core/src/test/java/org/apache/helix/controller/stages/TestRecoveryLoadBalance.java
##########
@@ -57,7 +57,10 @@
private final String STATE_MODEL = "statemodel";
private ClusterConfig _clusterConfig;
- @Test(dataProvider = "recoveryLoadBalanceInput")
+ // Disable the test since recovery rebalance is not able to block load
rebalance.
+ // TODO: if we decide to support that by replica level, we will have a
different config for that and this test can be
Review comment:
This is the pain point that we want to address with this change. I
suggest change the test to validate the reverse way. So you don't have to
disable the test, and the new change will be protected by more coverage.
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ResourceMonitor.java
##########
@@ -422,22 +440,42 @@ public long getMissingReplicaPartitionGauge() {
return _numLessReplicaPartitions.getValue();
}
+ @Deprecated
Review comment:
Given the assumption that user will always rely on the MBean Server, I
think we can directly remove these methods? They are now only for testing.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java
##########
@@ -889,17 +893,23 @@ private int getIdealStateMatched(Partition partition) {
private void computeIntermediateMap(PartitionStateMap intermediateStateMap,
Review comment:
To avoid duplicate code, how about generalizing computeIntermediateMap
so it takes only one map and always computes with the same logic?
for (Map.Entry<Partition, Map<String, Message>> entry :
inputMessageMap.entrySet()) {
entry.getValue().entrySet().stream().forEach(e -> {
if
(!e.getValue().getToState().equals(HelixDefinedState.DROPPED.name())) {
intermediateStateMap.setState(entry.getKey(),
e.getValue().getTgtName(), e.getValue().getToState());
} else {
intermediateStateMap.getStateMap().get(entry.getKey()).remove(e.getValue().getTgtName());
}
});
}
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ResourceMonitor.java
##########
@@ -370,10 +388,10 @@ public void updateStateHandoffStats(MonitorState
monitorState, long totalDuratio
public void updateRebalancerStats(long numPendingRecoveryRebalancePartitions,
long numPendingLoadRebalancePartitions, long
numRecoveryRebalanceThrottledPartitions,
long numLoadRebalanceThrottledPartitions) {
-
_numPendingRecoveryRebalancePartitions.updateValue(numPendingRecoveryRebalancePartitions);
-
_numPendingLoadRebalancePartitions.updateValue(numPendingLoadRebalancePartitions);
-
_numRecoveryRebalanceThrottledPartitions.updateValue(numRecoveryRebalanceThrottledPartitions);
-
_numLoadRebalanceThrottledPartitions.updateValue(numLoadRebalanceThrottledPartitions);
+
_numPendingRecoveryRebalanceReplicas.updateValue(numPendingRecoveryRebalancePartitions);
Review comment:
Why leave all the deprecated fields? They will be zero forever, right?
I prefer removing them. Or if strict backward compatibility is to be
ensured, then just keep updating their values.
##########
File path:
helix-core/src/test/java/org/apache/helix/controller/stages/TestIntermediateStateCalcStage.java
##########
@@ -25,12 +25,14 @@
import java.util.Map;
import com.google.common.collect.ImmutableList;
+import java.util.UUID;
Review comment:
nit, not used?
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java
##########
@@ -467,7 +467,8 @@ private void chargePendingTransition(Resource resource,
CurrentStateOutput curre
for (Message message : pendingMessages) {
StateTransitionThrottleConfig.RebalanceType rebalanceType =
getRebalanceTypePerMessage(requiredStates, message,
currentStateMap);
- String currentState = currentStateMap.get(message.getTgtName());
+ String currentState = currentStateMap.get(message.getTgtName()) ==
null ? stateModelDefinition.getInitialState()
+ : currentStateMap.get(message.getTgtName());
Review comment:
nit, how about just add a null check like this, I think it's simpler and
sightly more efficient.
String currentState = currentStateMap.get(message.getTgtName());
if (currentState == null) {
currentState = stateModelDefinition.getInitialState();
}
--
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]
