jiajunwang commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486723813
##########
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:
This depends on the mapping calculator implementation. There is no
guarantee. So we'd better keep the check.
##########
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:
Yeah, this is a behavior change. But the original behavior is the
problem here. If any value is set in the output, then the later stage will just
use without any doubt. This is very dangerous.
I checked later stages like intermediate state calc, and message generating,
they can handle a null output gracefully. But they cannot handle the output
with the wrong information gracefully.=
##########
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:
Yeah, this is a behavior change. But the original behavior is the
problem here. If any value is set in the output, then the later stages will
just use it without any doubt. This is very dangerous.
I checked later stages like intermediate state calc, and message generating,
they can handle a null output gracefully. But they cannot handle the output
with the wrong information gracefully.=
##########
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:
Yeah, this is a behavior change. But the original behavior is the
problem here. If any value is set in the output, then the later stages will
just use it without any doubt. This is very dangerous.
I checked later stages like intermediate state calc, and message generating,
they can handle a null output gracefully. But they cannot handle the output
with the wrong information gracefully.
##########
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:
The original code returns false while the output is filled with an empty
mapping. However, this empty mapping is misleading.
##########
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:
The original code returns false while the output is added with an empty
assignment mapping item. However, this empty mapping is misleading.
----------------------------------------------------------------
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]
