Refactor leave - balance and tail violations #972
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Never attempt a simple transfer, always rebalance when a cluster leave operation is requested
sequential_claim is the refactored version not the v1 version (which should handle tail violations).
Prefer to transfer to a node with a lower current count of vnodes - so that by default simple_transfer will tend to give more balanced results
Also include warning in the plan output
Reviewed previous code, to ensure this is being handled correctly. However, this does appear to be incorrect - as the current simple_transfer does not handle tail violations. It only checks forward to the end of the list for TargetN violation - it does not wrap around to the start of the list.
Initial unit test added
Mas i970 simpleleavebalanced
ThomasArts
reviewed
Oct 4, 2021
ThomasArts
reviewed
Oct 4, 2021
ThomasArts
reviewed
Oct 4, 2021
src/riak_core_gossip.erl
Outdated
| force_rebalance; | ||
| false -> | ||
| TargetN = app_helper:get_env(riak_core, target_n_val), | ||
| Owners = riak_core_ring:all_owners(Ring), |
There was a problem hiding this comment.
This is almost the same code as in line 383 and following
There was a problem hiding this comment.
Refactored so Owners/Members are determined only once
ThomasArts
reviewed
Oct 5, 2021
ThomasArts
approved these changes
Oct 7, 2021
Just to explicitly state the requirement that input to the function (Owners) must be sorted. This is a common assumption in riak_core, and was an assumption of the replaced function.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some curious and unexpected cluster plans when running leave operations, prompted a review of the lead code.
This PR corrects a number of issues found:
The first part of a leave plan is to plan to handoff all vnodes on the leaving nodes to other members of the cluster, in a way that does not break the target_n_val. The logic of this attempt_simple_leave function was incorrect, as it would produce plans that would include tail violations (where the location was safe when the ring was viewed as a list, but not when it was considered as a ring where the head wraps round to the tail). This issue is corrected.
The attempt_simple_leave did not attempt to balance the vnodes in any way, leading in some cases to unbalanced plans (and leaves that might not be supportable as a receiving node may not have sufficient space). There is now a preference to make choices that lead to more balanced outcomes - and a workaround to skip this stage and move straight to a freshly balanced cluster plan (
riak_core.full_rebalance_onleave).When a full rebalance was forced, as it was not possible to produce a safe outcome to
attempt_simple_transfer, then a full rebalance would be triggered. However a deprecated function was used, one which could lead to tail violations (ignoring the improvements made in Riak 2.2.5). The updated function is now used.