You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/09/11 00:58:07 UTC
[GitHub] [helix] jiajunwang opened a new pull request #1359: Don't output the best possible state calculate result if the result is not valid.
jiajunwang opened a new pull request #1359:
URL: https://github.com/apache/helix/pull/1359
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1358
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
The invalid result may mislead the following stages in the pipeline and cause problems. For example, this change fixes the issue that resource monitor rebalance state may be inaccurately reported.
### Tests
- [X] The following tests are written for this issue:
Modified the existing test to cover change.
- [X] The following is the result of the "mvn test" command on the appropriate module:
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestDeleteJobFromJobQueue.testForceDeleteJobFromJobQueue:75 ยป Helix Failed to ...
[INFO]
[ERROR] Tests run: 1200, Failures: 1, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:14 h
[INFO] Finished at: 2020-09-10T17:51:36-07:00
[INFO] ------------------------------------------------------------------------
Rerun
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 8.305 s - in org.apache.helix.integration.task.TestDeleteJobFromJobQueue
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 14.473 s
[INFO] Finished at: 2020-09-10T17:56:04-07:00
[INFO] ------------------------------------------------------------------------
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1359:
URL: https://github.com/apache/helix/pull/1359#issuecomment-692870429
This PR has been approved and ready to be merged.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486781796
##########
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[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.
Posted by 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.
##########
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
[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.
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486717562
##########
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[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.
Posted by 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`?
##########
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[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.
Posted by 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
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486724878
##########
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.=
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486781796
##########
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.
##########
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.
##########
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.
##########
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.
##########
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.
##########
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.
##########
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.
##########
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.
##########
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[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.
Posted by 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:
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
[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.
Posted by 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
[GitHub] [helix] jiajunwang merged pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1359:
URL: https://github.com/apache/helix/pull/1359
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486724878
##########
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.=
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486781796
##########
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.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1359: Don't output the best possible state calculate result if the result is not valid.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1359:
URL: https://github.com/apache/helix/pull/1359#discussion_r486724878
##########
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.
----------------------------------------------------------------
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