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.

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

Allow partitionAssignment API in maintenance mode #2742

Merged

Conversation

GrantPSpencer
Copy link
Contributor

Issues

Currently we do not return a partitionAssignment result if the cluster is in maintenance mode.

Allowing this would enable customers to make a series of changes to their cluster while in maintenance and then query the partitionAssignment one single time to get the expected assignments, without each change to the cluster triggering a rebalance.

Description

Allow partitionAssignment API to calculate an assignment while the cluster is in maintenance mode. This also includes a minor change to WAGED rebalancer in BestPossibleCalcStage, as computeResourceBestPossibleStateWithWagedRebalancer currently returns an empty map if the cluster is in maintenance mode.

Tests

  • The following tests are written for this issue:

testComputePartitionAssignmentMaintenanceMode

Changes that Break Backward Compatibility (Optional)

  • My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:

N/A

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":
    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"

Code Quality

  • My diff has been formatted using helix-style.xml
    (helix-style-intellij.xml if IntelliJ IDE is used)

Copy link
Contributor

@zpinto zpinto left a comment

Choose a reason for hiding this comment

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

Overall LGTM 👍
Some minor formatting comments.

@GrantPSpencer
Copy link
Contributor Author

Overall LGTM 👍 Some minor formatting comments.

Good eye, fixed these in recent commit. Thanks!

@GrantPSpencer
Copy link
Contributor Author

@xyuanlu can you re-run PR CI? The failing test failed 2 weeks ago as well, may be flaky
#2731

@xyuanlu xyuanlu merged commit f193045 into apache:master Jan 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants