-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Empty IdealState Calculation with Disabled Nodes in AutoRebalance… #2877
Fix Empty IdealState Calculation with Disabled Nodes in AutoRebalance… #2877
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I suggest separate the logic change and style change? It is easier to review and track change history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split the PR with fix + refactor.
...ore/src/main/java/org/apache/helix/controller/rebalancer/strategy/AutoRebalanceStrategy.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/org/apache/helix/controller/rebalancer/strategy/AutoRebalanceStrategy.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/org/apache/helix/controller/rebalancer/strategy/AutoRebalanceStrategy.java
Show resolved
Hide resolved
...ore/src/main/java/org/apache/helix/controller/rebalancer/strategy/AutoRebalanceStrategy.java
Outdated
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/controller/rebalancer/TestAutoRebalanceStrategy.java
Outdated
Show resolved
Hide resolved
let @zpinto take another look? |
@MarkGaox can you follow up with Zac? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR is ready to merge. Approved by @junkaixue @zpinto Commit message: Fix IdealState Calculation in AutoRebalanceStrategy for Disabled Nodes |
…Strategy
Issues
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
Description
(Write a concise description including what, why, how)
When calculating the preferred assignment, we should skip the node if the node is already disabled.
Tests
mvn clean install -Dmaven.test.skip.exec=true && mvn test -o -Dtest=TestAutoRebalanceStrategy -pl=helix-core
Changes that Break Backward Compatibility (Optional)
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
Documentation (Optional)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)