Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement the WAGED rebalance with delayed rebalance logic. #1

Closed
wants to merge 1 commit into from
Closed

Implement the WAGED rebalance with delayed rebalance logic. #1

wants to merge 1 commit into from

Conversation

jiajunwang
Copy link
Owner

@jiajunwang jiajunwang commented Aug 23, 2019 •
edited
Loading

Issues

  • My PR addresses the following Helix issues and references them in the PR title:

apache#402

Description

  • Here are some details about my PR, including screenshots of any UI changes:

This PR implements the WAGED rebalancer.
Delayed rebalance logic will be integrated with the WAGED rebalancer later.

Tests

  • The following tests are written for this issue:

TBD

  • The following is the result of the "mvn test" command on the appropriate module:

TBD

Commits

  • 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":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation in the following wiki page:

https://github.com/apache/helix/wiki/Weight-aware-Globally-Evenly-distributed-Rebalancer

Code Quality

  • My diff has been formatted using helix-style.xml

i3wangyi
i3wangyi reviewed Aug 23, 2019
View reviewed changes
helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java
@@ -41,18 +60,279 @@
public class WagedRebalancer implements GlobalRebalancer<ResourceControllerDataProvider> {
private static final Logger LOG = LoggerFactory.getLogger(WagedRebalancer.class);

// When any of the following change happens, the rebalancer needs to do a global rebalance.
private static final Set<HelixConstants.ChangeType> GLOBAL_REBALANCE_REQUIRED_CHANGE_TYPES =
Collections.unmodifiableSet(new HashSet<>(Arrays
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ImmutableSet.of

helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java Outdated
private AssignmentMetadataStore _assignmentMetadataStore;
private RebalanceAlgorithm _rebalanceAlgorithm;
private MappingCalculator<ResourceControllerDataProvider> _mappingCalculator;

@Override
public void init() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the init method is necessary. If the class is stateless we can create singleton instance. A constructor will give you the ability to mock the dependency for unit testing

helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/WagedRebalancer.java Outdated
public Map<String, IdealState> computeNewIdealStates(ResourceControllerDataProvider clusterData,
Map<String, Resource> resourceMap, final CurrentStateOutput currentStateOutput) {
return new HashMap<>();
public Map<String, IdealState> computeNewIdealStates(ResourceControllerDataProvider dataProvider,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The public method and the related private methods are too long and unbelievably difficult to understand and maintain

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have divided the PR to 2 pieces. But in general, this logic is one of the most complicated ones in our project. That's why we need to start early.
And, believe me, split them into sub methods won't help with readability...

Please try to understand what we are doing and comment with detail if you think any part can be simplified.

@jiajunwang jiajunwang force-pushed the integrate branch 2 times, most recently from a507dc8 to 91456f8 Compare August 23, 2019 22:28
Implement basic version of the WAGED rebalancer.
8639c03 
Note this implementation does not contain delayed rebalance logic.
Unit test to be added.
@jiajunwang jiajunwang closed this Aug 29, 2019
@jiajunwang jiajunwang mentioned this pull request Aug 3, 2020
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@i3wangyi i3wangyi i3wangyi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jiajunwang @i3wangyi