[GitHub] [helix] rahulrane50 commented on a diff in pull request #2340: Add metrics for rebalance throttled because of error partition

[GitBox <gi...@apache.org>]

rahulrane50 commented on code in PR #2340:
URL: https://github.com/apache/helix/pull/2340#discussion_r1073928319


##########
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ResourceMonitor.java:
##########
@@ -128,6 +129,8 @@ public ResourceMonitor(String clusterName, String resourceName, ObjectName objec
     _numOfPartitionsInExternalView = new SimpleDynamicMetric("ExternalViewPartitionGauge", 0L);
     _numOfPartitions = new SimpleDynamicMetric("PartitionGauge", 0L);
     _numPendingStateTransitions = new SimpleDynamicMetric("PendingStateTransitionGauge", 0L);
+    _rebalanceThrottledByErrorPartitionGauge =

Review Comment:
   I think you missed to reset it's value in resetResourceStateGauges() method?



##########
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ResourceMonitor.java:
##########
@@ -371,11 +374,12 @@ public void updateStateHandoffStats(MonitorState monitorState, long totalDuratio
 
   public void updateRebalancerStats(long numPendingRecoveryRebalancePartitions,
       long numPendingLoadRebalancePartitions, long numRecoveryRebalanceThrottledPartitions,
-      long numLoadRebalanceThrottledPartitions) {
+      long numLoadRebalanceThrottledPartitions, boolean rebalanceThrottledByErrorPartitio) {

Review Comment:
   neat typo: rebalanceThrottledByErrorPartitions



##########
helix-core/src/test/java/org/apache/helix/controller/stages/TestIntermediateStateCalcStage.java:
##########
@@ -272,6 +273,128 @@ public void testWithClusterConfigChange() {
     }
   }
 
+  @Test
+  public void testThrottleByErrorPartition() {
+    String resourcePrefix = "resource";
+    int nResource = 3;
+    int nPartition = 3;
+    int nReplica = 3;
+
+    String[] resources = new String[nResource];
+    for (int i = 0; i < nResource; i++) {
+      resources[i] = resourcePrefix + "_" + i;
+    }
+
+    preSetup(resources, nReplica, nReplica);
+    event.addAttribute(AttributeName.RESOURCES.name(),
+        getResourceMap(resources, nPartition, "OnlineOffline"));
+    event.addAttribute(AttributeName.RESOURCES_TO_REBALANCE.name(),
+        getResourceMap(resources, nPartition, "OnlineOffline"));
+    ClusterStatusMonitor monitor = new ClusterStatusMonitor(_clusterName);
+    monitor.active();
+    event.addAttribute(AttributeName.clusterStatusMonitor.name(), monitor);
+
+    // Initialize best possible state and current state
+    BestPossibleStateOutput bestPossibleStateOutput = new BestPossibleStateOutput();
+    MessageOutput messageSelectOutput = new MessageOutput();
+    CurrentStateOutput currentStateOutput = new CurrentStateOutput();
+    IntermediateStateOutput expectedResult = new IntermediateStateOutput();
+
+    _clusterConfig.setErrorOrRecoveryPartitionThresholdForLoadBalance(0);

Review Comment:
   For my understanding, we have set this value 0 here mean there always be downward rebalance because all positive values values will be greater than 0. If that's the correct understanding then don't we want to test for positive threshold value?



-- 
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-unsubscribe@helix.apache.org

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