Refactor WagedRebalancer and add comments#2431
Conversation
Create standalone classes for partial and global rebalance to make the class more modular and easier to manage.
desaikomal
left a comment
There was a problem hiding this comment.
This is extremely good change. Splitting the functionality is very useful.
It is not possible to look through each and every line of change as the change is really big for human brain to perform.
Hopefully lift-n-shift worked. AFAIK you didn't fundamentally changed any behavior. just code alignments.
Amazingly cool
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentManager.java
Show resolved
Hide resolved
...x-core/src/main/java/org/apache/helix/controller/rebalancer/waged/GlobalRebalanceRunner.java
Show resolved
Hide resolved
|
This PR is ready to merge, approved by @junkaixue Refactor WagedRebalancer and add comments |
rahulrane50
left a comment
There was a problem hiding this comment.
This looks very clean! Good job Quincy! I just wish github has"moved code block" feature. it would have been very easy to make sure we don't miss anything in refactoring :)
| * assignmentMetadataStore, return the current state assignment. | ||
| * @throws HelixRebalanceException | ||
| */ | ||
| public Map<String, ResourceAssignment> getBestPossibleAssignment( |
There was a problem hiding this comment.
nit. Actually these two methods are almost identical except "getBaseline()" vs "getBestPossibleAssignment()" call. I totally understand if we want keep both of them but just wondering if changing signature of this method (AssignmentMetadataStore --> func) might help to combine both of this methods.
There was a problem hiding this comment.
Thanks for the review.
That's a good idea should be doable, but let's leave that for future refactoring as this PR is big enough.
|
Thanks for making the effort on code clean and refactor! |
Refactor WagedRebalancer and add comments Create standalone classes for partial and global rebalance to make the class more modular and easier to manage.
Create standalone classes for partial and global rebalance to make the class more modular and easier to manage.
Issues
Resolves #2430
Description
Refactor
WagedRebalancerby separating out components to their standalone classes.Currently the WagedRebalancer class is complicated and contains multiple processes. With everything in one class, it's hard to read and even worse to manage complexity.
Creating their classes for partial and global rebanace have a clear cut on responsibility, and make it possible for future refactoring toward microservices.
This PR does the following:
No new logic is introduced, this PR is pure refactoring.
Tests
Existing tests in
TestWagedRebalancer(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
Test for
TestWagedRebalancer[INFO] Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.535 s - in org.apache.helix.controller.rebalancer.waged.TestWagedRebalancer
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 12, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco-maven-plugin:0.8.6:report (generate-code-coverage-report) @ helix-core ---
[INFO] Loading execution data file /Users/qqu/workspace/qqu-helix/helix-core/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Core' with 936 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 40.842 s
[INFO] Finished at: 2023-04-03T15:42:46-04:00
[INFO] ------------------------------------------------------------------------
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)