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

Fix endless creation of best possible nodes #2970

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GrantPSpencer
Copy link
Contributor

@GrantPSpencer GrantPSpencer commented Nov 26, 2024

Issues

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Controller endlessly creates best possible nodes under certain circumstances due handlehandleDelayedRebalanceMinActiveReplica modifying the same underlying ResourceAssignments that are used for the in memory bestPossibleAssignment

This PR changes AssignmentMetadataStore's methods persistBaseline and persistBestPossibleAssignment to create a copy of the underlying ResourceAssignment objects before persisting them to the store's in-memory map. This allows the object passed to these methods to be safely modified afterwards without affecting the cached bestPossible.

There are specific circumstances to trigger this behavior:

  1. Cluster is in state to require delyRebalance overwrites (nodes are disabled within delay window and partitions are violating minActiveReplica count)
  2. Change cluster so that partialRebalance will calculate a new assignment (adding or removing instances)

PartialRebalance will async calculate a new best possible and increment the _bestPossibleVersion and schedule an onDemandPipeline run. In emergencyRebalance(...) of the next pipeline, the bestPossibleAssignment (object A) will be persisted as _bestPossibleVersion != _lastPersistedBestPossibleVersion. The values of the new best possible are stored in our in-memory bestPossibleAssignment map (object B). While object A and B are different, their values map to the same objects. We then pass object A to handleDelayedRebalanceMinActiveReplica which modifies the values of object A, also modifying the values of object B. This causes partialRebalance to believe it has calculated a different assignment, so it triggers a new pipeline run which will repeat the above behavior ad infinitum.

Tests

  • The following tests are written for this issue:

TestEndlessBestPossibleNodes
This test will fail on master, but pass on this branch

$ mvn test -o -Dtest=TestEndlessBestPossibleNodes -pl=helix-core 

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 15.512 s - in org.apache.helix.integration.TestEndlessBestPossibleNodes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  53.849 s
[INFO] Finished at: 2024-11-26T15:52:41-08:00
[INFO] ------------------------------------------------------------------------

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)

@GrantPSpencer GrantPSpencer force-pushed the another-best-possible-bug branch from 2690482 to e22639d Compare November 26, 2024 23:51
@GrantPSpencer GrantPSpencer marked this pull request as ready for review November 27, 2024 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant