kvserver: tighten and de-fatal replicaGC checks #102248
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
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
tbg
commented
Apr 25, 2023
tbg
commented
Jun 23, 2023
Now we return most failed checks as errors and report errors to sentry (but
without crashing). `TestStoreRemoveReplicaDestroy` was updated with somewhat
better coverage of these error conditions, which is now
I also slightly rearranged the checks; it makes sense to me to check that the
store doesn't have a different Replica, rather than using a potentially old
Replica and failing the nextReplicaID check.
old order new order
- assert replica is inited - assert replica is inited
- assert opts are compatible with each other - assert opts are compatible with each other
- maybe return early - maybe return early
- assert destroy status - assert destroy status
- assert replica id < nextReplicaID ───┐ ┌─► - *assert same replica in store*
- assert replica is inited (again) └──┼─► - *assert replica id < nextReplicaID*
- set destroy status │ - set destroy status
- assert same replica in store────────────┘ - remainder
- remainder
Closes cockroachdb#94813.
Epic: None
Release note: None
|
TFTR! bors r=erikgrinaker |
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 5, 2023
Noticed in cockroachdb#102248. Epic: none Release note: None
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Jul 5, 2023
106103: kvserver: fix off-by-one in FlushLockedWithRaftGroup r=erikgrinaker a=tbg We need to skip the conf change itself. The result of this bug was that proposals and entries wouldn't be lined up correctly if there was a conf change in the batch. Luckily, the impact of that is limited, since all the proposals are used for in `proposeBatch` was to log an event to the context. Still, good to fix and improve test coverage. Epic: CRDB-25287 Release note: None 106141: logcrash: log Error, not Warning r=knz,erikgrinaker a=tbg Noticed in #102248. Epic: none Release note: None Co-authored-by: Tobias Grieger <[email protected]>
blathers-crl bot
pushed a commit
that referenced
this pull request
Jul 5, 2023
Noticed in #102248. Epic: none Release note: None
blathers-crl bot
pushed a commit
that referenced
this pull request
Jul 5, 2023
Noticed in #102248. Epic: none Release note: None
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.
Now we return most failed checks as errors and report errors to sentry (but
without crashing).
Closes #94813.
Epic: None
Release note: None