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

kvserver: update AdminRelocateRange to use explicit swaps of voters and non-voters #62370

Closed
aayushshah15 opened this issue Mar 22, 2021 · 2 comments · Fixed by #62631
Closed
Assignees
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-kv-replication-constraints GA-blocker

Comments

@aayushshah15
Copy link
Contributor

AdminRelocateRange currently doesn’t make use of the ability to promote non-voters to voters, demote voters to non-voters or swap voters and non-voters. Because of this limitation, the mergeQueue has to resort to a fairly complex way of determining which targets to relocate a range’s replicas to during a merge in order to avoid redundant data movement.

There are a few problems with this:

  • With the ability to swap replica types, the complexity mentioned above is unnecessary. We can avoid redundant data movement by performing all the promotions / demotions / swaps and then relocating the rest of the misaligned replicas.
  • The merge queue relocation code was written before config: introduce num_voters and voter_constraints #57184. We’ve since introduced the concept of voter_constraints which govern the constraints over just the set of voting replicas. This means that the relocation algorithm computes results that may not necessarily meet the {voter_}constraints placed over the RHS. In such cases, the range merge will fail.
  • It also has this limitation:
    if len(finalRightVoters.Descriptors()) == 0 {
    // TODO(aayush): We can end up in this case for scenarios like the
    // following (the digits represent StoreIDs):
    //
    // LHS-> voters: {1, 2, 3}, non-voters: {}
    // RHS-> voters: {4}, non-voters: {1, 2, 3}
    //
    // Remove this error path once we support swapping voters and non-voters.
    return nil, nil,
    errors.UnimplementedErrorf(errors.IssueLink{IssueURL: build.MakeIssueURL(58499)},
    "unsupported configuration of RHS(%s) and LHS(%s) as it requires an atomic swap of a"+
    " voter and non-voter", rightRepls, leftRepls)
    }

We need to update AdminRelocateRange to use the aforementioned voter<->non-voter swaps and simply have the merge queue directly call into it, instead of having it perform its own calculation of where to move the {voter,non-voter} replicas.

@aayushshah15 aayushshah15 added A-kv-replication Relating to Raft, consensus, and coordination. A-kv-replication-constraints GA-blocker labels Mar 22, 2021
@aayushshah15 aayushshah15 self-assigned this Mar 22, 2021
@blathers-crl
Copy link

blathers-crl bot commented Mar 22, 2021

Hi @aayushshah15, please add a C-ategory label to your issue. Check out the label system docs.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@aayushshah15
Copy link
Contributor Author

cc @nvanbenschoten and @andreimatei so you're aware. This fell off my radar because I didn't create a tracking issue before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-kv-replication-constraints GA-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant