Re-introduce "kv,migration: rm code handling legacy raft truncated st… #70464
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
|
This change is |
ajwerner
reviewed
Sep 21, 2021
There was a problem hiding this comment.
Sorry for the dance and thank you! I know there was toil.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @tbg)
…ate" This reverts commit ef1dd6f. cockroachdb#70432 reverted cockroachdb#69887, as temporary stop-gap until we release the first 21.2 beta. See the discussion over on cockroachdb#70432 for why we want to queue up this revert to the original revert; this should only be merged after cockroachdb#69826 lands. Release note: None
irfansharif
force-pushed
the
210920.reintroduce-trunc-state
branch
from
cfd9d5a to
4df8ac2
Compare
|
Rebased now that #69826 landed, this should be safe to merge (I think). Can I has a rubber stamp? |
erikgrinaker
approved these changes
Oct 26, 2021
|
Thanks Erik! bors r+ |
|
Build failed: |
Unrelated flake. Looks like this test isn't expecting non-contiguous store IDs. bors r+ |
|
Build succeeded: |
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Oct 29, 2021
Fixes cockroachdb#72029. using_applied_state_key was previously used to check whether the Range had upgraded to begin using the RangeAppliedState key. When set to true, replicas receiving the state (through a ReplicatedEvalResult) knew to begin using the new key. In cockroachdb#58088 (in 21.1) we introduced a migration to iterate through all ranges in the system and have them start using the RangeAppliedState key. In 21.2, this field was always set to true -- 21.2 nodes were already using the RangeAppliedState key, or receiving messages from 21.1 nodes that were also using the RangeAppliedState key. In 21.2 (and in 21.1 for that matter) we didn't need to trigger the "if set to true in an incoming message, start using the RangeAppliedState key" code path. When looking to get rid of this field in 22.1 (cockroachdb#70464), we observed that there was an unintentional read of this field in 21.2 nodes (see cockroachdb#72029 and \cockroachdb#72222); the saga is as follows: - Removing this field in 22.1 meant it was set as false when received at 21.2 nodes. - This should've been fine! We weren't using this field to trigger any upgrade code paths (like it was originally intended for). - It turns out that in 21.2 we were using the ReplicaState from the incoming snapshot to update our in-memory replica state - Because the proto field was being phased out, there was now a divergence between the on-disk state (field set to true, from earlier 21.2 operations) and the in-memory state (field set to false, because sent from a version that attempted to get rid of this field). Removing proto fields from the replica state are not possible until we stop using the protobuf copy of the replica state when applying a snapshot (cockroachdb#72222). Once that's done, we should be able to stop sending the replica state as part of the snapshot in the subsequent release. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Oct 29, 2021
72132: coldata: fix BenchmarkAppend so that it could be run with multiple count r=yuzefovich a=yuzefovich Previously, we could get an index out of bounds because we'd allocate larger `Bytes.data` slice than `int32` offset can support. Release note: None 72239: kvserver: resurrect using_applied_state_key in ReplicaState r=irfansharif a=irfansharif Fixes #72029. `using_applied_state_key` was previously used to check whether the Range had upgraded to begin using the `RangeAppliedState` key. When set to true, replicas receiving the state (through a `ReplicatedEvalResult`) knew to begin using the new key. In #58088 (in 21.1) we introduced a migration to iterate through all ranges in the system and have them start using the `RangeAppliedState` key. In 21.2, this field was always set to true -- 21.2 nodes were already using the `RangeAppliedState` key, or receiving messages from 21.1 nodes that were also using the `RangeAppliedState` key. In 21.2 (and in 21.1 for that matter) we didn't need to trigger the "if set to true in an incoming message, start using the `RangeAppliedState` key" code path. When looking to get rid of this field in 22.1 (#70464), we observed that there was an unintentional read of this field in 21.2 nodes (see #72029 and \#72222); the saga is as follows: - Removing this field in 22.1 meant it was set as false when received at 21.2 nodes. - This should've been fine! We weren't using this field to trigger any upgrade code paths (like it was originally intended for). - It turns out that in 21.2 we were using the `ReplicaState` from the incoming snapshot to update our in-memory replica state - Because the proto field was being phased out, there was now a divergence between the on-disk state (field set to true, from earlier 21.2 operations) and the in-memory state (field set to false, because sent from a version that attempted to get rid of this field). Removing proto fields from the replica state are not possible until we stop using the protobuf copy of the replica state when applying a snapshot (#72222). Once that's done, we should be able to stop sending the replica state as part of the snapshot in the subsequent release. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: irfan sharif <[email protected]>
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.
…ate"
This reverts commit ef1dd6f. #70432
reverted #69887, as temporary stop-gap until we release the first 21.2
beta. See the discussion over on #70432 for why we want to queue up this
revert to the original revert; this should only be merged after #69826
lands.
Release note: None