kvserver: extract kvstorage.DestroyReplica#96650
Merged
craig[bot] merged 11 commits intocockroachdb:masterfrom Feb 8, 2023
Merged
kvserver: extract kvstorage.DestroyReplica#96650craig[bot] merged 11 commits intocockroachdb:masterfrom
craig[bot] merged 11 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
tbg
commented
Feb 6, 2023
tbg
commented
Feb 6, 2023
tbg
commented
Feb 6, 2023
90cceb9 to
fa7bd75
Compare
There are four callers to this method, and in all of them it is obvious that the replica is marked as destroyed. The invariant this code was checking is important, but also this code primarily stages updates into a batch. It is not actually necessary to have the replica marked as destroyed at this point yet, though in practice we do do it. What you really need is to prevent further modification of the data, but that still allows reads to be served until we actually commit the batch. Removing the dependency on `*Replica` will allow a series of simplifying refactors, at the end of which the end of the life of a Replica should be a little less nested, and a little more clear. There'll be a better place to assert this invariant as the work unfolds. Epic: CRDB-220 Release note: None
Prep for more refactoring. Epic: CRDB-220 Release note: None
It now checks in two ways that we're not regressing: 1. tombstone actually has to cover the replica's replicaID 2. tombstone itself must not regress. I had verified that all call sites at least claim to not regress and also conceptually I'm not aware of any that would want/need to rely on being forwarded by the old code. But there are lots of callers, so better to fail loudly in that case. If you see this commit, the `kvserver` tests have passed. Epic: CRDB-220 Release note: None
It now has to be there, so turn this into an assertion failure. See cockroachdb#95513. Epic: CRDB-220 Release note: None
Make this all about engine interactions. This removes the first assertion added in the last commit (tests had passed with it) and replaces it with a corresponding assertion on the persisted ReplicaID. Epic: CRDB-220 Release note: None
Epic: CRDB-220 Release note: None
Epic: CRDB-220 Release note: None
This is in preparation for moving the storage interactions of replica removal, too. Epic: CRDB-220 Release note: None
Epic: CRDB-220 Release note: None
It's now a first-class citizen of `kvstorage`, and we can now write better unit tests for it. Epic: CRDB-220 Release note: None
Epic: CRDB-220 Release note: None
fa7bd75 to
6b7de86
Compare
Member
Author
|
bors r=pavelkalinnikov |
Contributor
|
Build succeeded: |
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
3 participants
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 series of commits extracts
(*Replica).preDestroyRaftMuLockedinto afree-standing method
kvstorage.DestroyReplica.In doing so, it separates the in-memory and on-disk steps that are a part
of replica removal, and makes the on-disk steps unit testable.
Touches #93241.
Epic: CRDB-220
Release note: None