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`?
##########
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:
If `checkBestPossibleStateCalculation(idealState)` is true, will we ever
get a null `partitionStateAssignment`?
##########
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:
Is this to make sure if we ever get a null value for
partitionStateAssignment, we don't set anything in the output?
I wonder if this might cause a behavior change. Before this change, did
`output` get populated with some preferenceList value (maybe the one from the
previous, last successful pipeline)? After this change, if the
`computeBestPossiblePartitionState` returns a `null`, then output will start
having nothing, which is a different behavior from before this change?
----------------------------------------------------------------
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]
