[GitHub] [helix] narendly 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>]

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



##########
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;
         }

Review comment:
       Question: if `checkBestPossibleStateCalculation(idealState)` is true, will we ever get a null `partitionStateAssignment`? 




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