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/01/23 00:20:15 UTC
[GitHub] [helix] jiajunwang opened a new pull request #696: Add WAGED
rebalancer reset method to clean up cached status.
jiajunwang opened a new pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
#660
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
The reset method is for allowing to clean up any in-memory records within the rebalancer without recreating it.
Detailed change list:
1. Add reset methods to all the stateful objects that are used in the WAGED rebalancer.
2. Refine some of the potential race condition in the WAGED rebalancer components.
3. Adjust the tests accordingly. Also adding new tests to cover the components reset / the WAGED rebalancer reset logic.
### Tests
- [x] The following tests are written for this issue:
TestResourceChagneDetector.testResetSnapshot
TestAssignmentMetadataStore.testAssignmentCache
TestWagedRebalancer.testReset
- [x] The following is the result of the "mvn test" command on the appropriate module:
running.
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. 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"
### Documentation
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [x] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #696: Add
WAGED rebalancer reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696#discussion_r370309654
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -146,6 +146,17 @@ public boolean persistBestPossibleAssignment(
return true;
}
+ protected synchronized void resetCache() {
Review comment:
resetMetadataStore it is. Good suggestion.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #696: Add WAGED rebalancer
reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on issue #696: Add WAGED rebalancer
reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on issue #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696#issuecomment-577837681
This PR is ready to be merged, approved by @narendly
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #696: Add
WAGED rebalancer reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696#discussion_r369906596
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -146,6 +146,17 @@ public boolean persistBestPossibleAssignment(
return true;
}
+ protected synchronized void resetCache() {
Review comment:
Nit: I think `clearMetadataStore()` might be a better name for this method because caching isn't really an explicit feature? Or resetMetadataStore()?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #696: Add
WAGED rebalancer reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696#discussion_r370309654
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -146,6 +146,17 @@ public boolean persistBestPossibleAssignment(
return true;
}
+ protected synchronized void resetCache() {
Review comment:
Let's just call it reset()
AssignmentMetadataStore.resetMetadataStore() is redundent.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #696: Add
WAGED rebalancer reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696#discussion_r370309107
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##########
@@ -177,12 +172,13 @@ private WagedRebalancer(AssignmentMetadataStore assignmentMetadataStore,
_assignmentMetadataStore = assignmentMetadataStore;
_rebalanceAlgorithm = algorithm;
_mappingCalculator = mappingCalculator;
+ if (manager == null) {
+ LOG.warn("HelixManager is not provided. The rebalancer is not going to schedule for a future "
+ + "rebalance even when delayed rebalance is enabled.");
Review comment:
DelayedRebalance != delayed rebalance.
WAGED rebalancer also supports delayed rebalance feature. And this check will only impact this feature inside the WAGED rebalancer. We need to log it so when we debug we ensure if the delayed rebalance feature has been enabled or not.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #696: Add
WAGED rebalancer reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696#discussion_r370309107
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##########
@@ -177,12 +172,13 @@ private WagedRebalancer(AssignmentMetadataStore assignmentMetadataStore,
_assignmentMetadataStore = assignmentMetadataStore;
_rebalanceAlgorithm = algorithm;
_mappingCalculator = mappingCalculator;
+ if (manager == null) {
+ LOG.warn("HelixManager is not provided. The rebalancer is not going to schedule for a future "
+ + "rebalance even when delayed rebalance is enabled.");
Review comment:
DelayedRebalance != delayed rebalance.
WAGED rebalancer also supports delayed rebalance feature. We need to log it so when we debug we ensure if the delayed rebalance feature has been enabled or not.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #696: Add
WAGED rebalancer reset method to clean up cached status.
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #696: Add WAGED rebalancer reset method to clean up cached status.
URL: https://github.com/apache/helix/pull/696#discussion_r369897296
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
##########
@@ -177,12 +172,13 @@ private WagedRebalancer(AssignmentMetadataStore assignmentMetadataStore,
_assignmentMetadataStore = assignmentMetadataStore;
_rebalanceAlgorithm = algorithm;
_mappingCalculator = mappingCalculator;
+ if (manager == null) {
+ LOG.warn("HelixManager is not provided. The rebalancer is not going to schedule for a future "
+ + "rebalance even when delayed rebalance is enabled.");
Review comment:
Whether HelixManager is null or not is not and should not be a flag for DelayedRebalance. Why does the log specifically mention delayed rebalance here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org