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

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

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


##########
helix-core/src/main/java/org/apache/helix/controller/stages/IntermediateStateCalcStage.java:
##########
@@ -421,7 +419,8 @@ private PartitionStateMap computeIntermediatePartitionState(ResourceControllerDa
     if (clusterStatusMonitor != null) {
       clusterStatusMonitor
           .updateRebalancerStats(resourceName, messagesForRecovery.size(), messagesForLoad.size(),
-              messagesThrottledForRecovery.size(), messagesThrottledForLoad.size());
+              messagesThrottledForRecovery.size(), messagesThrottledForLoad.size(),
+              onlyDownwardLoadBalance);

Review Comment:
   Is  onlyDownwardLoadBalance same as ERROR partition count?



##########
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterStatusMonitor.java:
##########
@@ -937,8 +937,7 @@ private void unregisterAllJobs() {
     }
   }
 
-  // For test only
-  protected ResourceMonitor getResourceMonitor(String resourceName) {
+  public ResourceMonitor getResourceMonitor(String resourceName) {

Review Comment:
   any particular reason to make it public?



##########
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterStatusMonitor.java:
##########
@@ -1121,4 +1120,13 @@ public long getPendingStateTransitionGuage() {
     }
     return total;
   }
+
+  @Override
+  public long getRebalanceThrottledByErrorPartition() {
+    long total = 0;
+    for (Map.Entry<String, ResourceMonitor> entry : _resourceMonitorMap.entrySet()) {

Review Comment:
   i am not great with Java8 lambda, but can this be written using that??  Again, I am not expert.



##########
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ResourceMonitor.java:
##########
@@ -467,19 +475,13 @@ public void resetMaxTopStateHandoffGauge() {
         _numPendingLoadRebalanceReplicas,
         _numRecoveryRebalanceThrottledReplicas,
         _numLoadRebalanceThrottledReplicas,
-        _externalViewIdealStateDiff,
-        _successfulTopStateHandoffDurationCounter,
-        _successTopStateHandoffCounter,
-        _failedTopStateHandoffCounter,
-        _maxSinglePartitionTopStateHandoffDuration,
-        _partitionTopStateHandoffDurationGauge,
+        _externalViewIdealStateDiff, _successfulTopStateHandoffDurationCounter,
+        _successTopStateHandoffCounter, _failedTopStateHandoffCounter,
+        _maxSinglePartitionTopStateHandoffDuration, _partitionTopStateHandoffDurationGauge,
         _partitionTopStateHandoffHelixLatencyGauge,
-        _partitionTopStateNonGracefulHandoffDurationGauge,
-        _totalMessageReceived,
-        _totalMessageReceivedCounter,
-        _numPendingStateTransitions,
-        _rebalanceState
-    );
+        _partitionTopStateNonGracefulHandoffDurationGauge, _totalMessageReceived,
+        _totalMessageReceivedCounter, _numPendingStateTransitions, _rebalanceState,

Review Comment:
   earlier style was keeping metrics per-line, you are combining 2 per-line. any particular reason?



-- 
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