[GitHub] [helix] kaisun2000 commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.

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

kaisun2000 commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486745486



##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java
##########
@@ -347,36 +347,39 @@ private boolean computeSingleResourceBestPossibleState(ClusterEvent event,
       LogUtil.logError(logger, _eventId, "Error computing assignment for resource " + resourceName
           + ". no rebalancer found. rebalancer: " + rebalancer + " mappingCalculator: "
           + mappingCalculator);
-    }
-
-    if (rebalancer != null && mappingCalculator != null) {
-      ResourceAssignment partitionStateAssignment = null;
+    } else {
       try {
         HelixManager manager = event.getAttribute(AttributeName.helixmanager.name());
         rebalancer.init(manager);
         idealState =
             rebalancer.computeNewIdealState(resourceName, idealState, currentStateOutput, cache);
 
-        output.setPreferenceLists(resourceName, idealState.getPreferenceLists());
+        // Check if calculation is done successfully
+        if (!checkBestPossibleStateCalculation(idealState)) {
+          LogUtil.logWarn(logger, _eventId,
+              "The calculated idealState is not valid, resource: " + resourceName);
+          return false;
+        }
 
         // Use the internal MappingCalculator interface to compute the final assignment
         // The next release will support rebalancers that compute the mapping from start to finish
-        partitionStateAssignment = mappingCalculator
+        ResourceAssignment partitionStateAssignment = mappingCalculator
             .computeBestPossiblePartitionState(cache, idealState, resource, currentStateOutput);
 
         if (partitionStateAssignment == null) {
           LogUtil.logWarn(logger, _eventId,
-              "PartitionStateAssignment is null, resource: " + resourceName);
+              "The calculated partitionStateAssignment is null, resource: " + resourceName);
           return false;
         }
 
+        // Set the calculated result to the output.
+        output.setPreferenceLists(resourceName, idealState.getPreferenceLists());

Review comment:
       It seems to me that the original code, `return checkBestPossibleStateCalculation(idealState);` would still return false. Does it feed the wrong info in `output` to later stage? 
   
   The main thing is that the new code fixed the issue of missing metric for invalid idealState.




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