Fix claim tail violations and unbalanced rings#913
Merged
Conversation
This commit squashes together some fixes for riak_core claim. There is a full write up in docs/claim-fixes.md. In brief: In some situations riak_core_claim can produce rings that have tail violations. A tail violation is where some preflists (drawn from the tail of the ring) do not enforce the target-n-val property. One example is ring using the old claim diag for 16 vnodes and 5 nodes. Such a ring would have a tail of 1. The last two preflists from that ring would be [5, 1, 1] and [1, 1,2] both of which would mean >1 replica on a single node. These "tail-violations" are addressed. A further issue with claim was that distributing existing claim to a new node could result in an unbalanced ring. An example would be adding a 5th node to a 32 vnode ring of 4 nodes. The distribution went from [8, 8, 8, 8] to [6, 6, 6, 8, 6] . Before assigning vnodes claim calculates "deltas" as a measure of how many vnodes each node wants or should give up. Prior to the change, for this example the deltas were [2, 2, 2, 2, -6] which means "you can 2 partitions from each of 4 nodes in order to provide 6 on the 5th". Claim would be greedy and take all 2 from each of the first 3 nodes and stop. This fix balances the deltas before claim so that they always equal the wants of the claiming node(s). For the example the deltas would be [2, 2, 1, 1, -6] ensuring that the final balance of vnodes will be [6, 6, 7, 7, 6]. These "unbalanced rings" are fixed. NOTE: there may still be ways to get bad rings with "leave" commands, as this is still not tested
Contributor
Author
|
riak_tests pass. Dialyzer passes. |
1 similar comment
Contributor
Author
|
riak_tests pass. Dialyzer passes. |
Some mild renaming/refactoring for better reading (I got confused while figuring out dialyzer errors and these names made more sense when I re-read it.)
|
I got a couple of test errors from running make test to do with the btypes tests from brops - i merged in this pr and that fixed the failing tests. All tests passing! Dialyzer also passes |
Contributor
Author
|
Is that +1 @nickkeers ? |
|
Not from me i'm afraid, I don't think I know enough about riak_core to comment on this one sadly. But I wanted to mention I've ran it and tests pass etc |
+1 from me |
This file contains hidden or 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
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.
This PR has some fixes for riak_core claim. There is a full write up in docs/claim-fixes.md.
In brief: In some situations riak_core_claim can produce rings that have tail violations. A tail violation is where some preflists (drawn from the tail of the ring) do not enforce the
target-n-valproperty. One example is ring using the old claim diag for 16 vnodes and 5 nodes. Such a ring would have a tail of1. The last two preflists from that ring would be[5, 1, 1]and[1, 1,2]both of which would mean >1 replica on a single node.A further issue with claim was that distributing existing claim to a new node could result in an unbalanced ring. An example would be adding a 5th node to a 32 vnode ring of 4 nodes. The distribution went from
[8, 8, 8, 8]to[6, 6, 6, 8, 6]. Before assigning vnodes claim calculates "deltas" as a measure of how many vnodes each node wants or should give up. Prior to the change, for this example the deltas were[2, 2, 2, 2, -6]which means "you can 2 partitions from each of 4 nodes in order to provide 6 on the 5th". Claim would be greedy and take all 2 from each of the first 3 nodes and stop. This fix balances the deltas before claim so that they always equal the wants of the claiming node(s). For the example the deltas would be[2, 2, 1, 1, -6]ensuring that the final balance of vnodes will be[6, 6, 7, 7, 6]