storage: continue sending ReplicaState.TxnSpanGCThreshold to 19.1 nodes #39003
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 |
Fixes cockroachdb#38996. We saw in the referenced issue that a 19.1 node crashed after being sent a snapshot with a TxnSpanGCThresholdKey but without the corresponding value in SnapshotRequest_Header.ReplicaState.TxnSpanGCThreshold. This commit avoids this assertion failure by continuing to send this field in the snapshot header, even though it is no longer maintained. 19.2 nodes will ignore the field during entry application and during snapshot ingestion, so the change has no effect on them. However, we can rest assured that the same assertion would fire if we messed this up on 19.2 nodes. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/fix38996
branch
from
b911b74 to
0ff7569
Compare
ajwerner
approved these changes
Jul 19, 2019
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
|
bors r=ajwerner |
craig bot
pushed a commit
that referenced
this pull request
Jul 20, 2019
39003: storage: continue sending ReplicaState.TxnSpanGCThreshold to 19.1 nodes r=ajwerner a=nvanbenschoten Fixes #38996. We saw in the referenced issue that a 19.1 node crashed after being sent a snapshot with a TxnSpanGCThresholdKey but without the corresponding value in SnapshotRequest_Header.ReplicaState.TxnSpanGCThreshold. This commit avoids this assertion failure by continuing to send this field in the snapshot header, even though it is no longer maintained. 19.2 nodes will ignore the field during entry application and during snapshot ingestion, so the change has no effect on them. However, we can rest assured that the same assertion would fire if we messed this up on 19.2 nodes. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Jul 29, 2019
…missing Fixes cockroachdb#39138. This was missed in cockroachdb#39003. v19.1 nodes populate their ReplicaState with an empty TxnSpanGCThreshold when the key is missing, not a nil TxnSpanGCThreshold. See https://github.com/cockroachdb/cockroach/blob/b8554ec29fd1620c0e6af9544d678db57d251f4c/pkg/storage/stateloader/stateloader.go#L474. We need to do the same to avoid an assertion failure after a snapshot. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Jul 29, 2019
39145: storage: add empty DeprecatedTxnSpanGCThreshold to snapshot when key missing r=nvanbenschoten a=nvanbenschoten Fixes #39138. This was missed in #39003. v19.1 nodes populate their ReplicaState with an empty TxnSpanGCThreshold when the key is missing, not a nil TxnSpanGCThreshold. See https://github.com/cockroachdb/cockroach/blob/b8554ec29fd1620c0e6af9544d678db57d251f4c/pkg/storage/stateloader/stateloader.go#L474-L482 We need to do the same to avoid an assertion failure after a snapshot. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]>
nvanbenschoten
added a commit
that referenced
this pull request
Jul 30, 2019
…missing Fixes #39138. This was missed in #39003. v19.1 nodes populate their ReplicaState with an empty TxnSpanGCThreshold when the key is missing, not a nil TxnSpanGCThreshold. See https://github.com/cockroachdb/cockroach/blob/b8554ec29fd1620c0e6af9544d678db57d251f4c/pkg/storage/stateloader/stateloader.go#L474. We need to do the same to avoid an assertion failure after a snapshot. Release note: None
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Dec 19, 2019
Completes the migration started in cockroachdb#38817 and partially preserved in cockroachdb#39003. This needed to wait for v20.1. Release note: None
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Dec 20, 2019
Completes the migration started in cockroachdb#38817 and partially preserved in cockroachdb#39003. This needed to wait for v20.1. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Dec 20, 2019
43328: storage: delete ReplicaState.DeprecatedTxnSpanGCThreshold r=nvanbenschoten a=nvanbenschoten Completes the migration started in #38817 and partially preserved in #39003. This needed to wait for v20.1. Release note: None Co-authored-by: Nathan VanBenschoten <[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.
Fixes #38996.
We saw in the referenced issue that a 19.1 node crashed after being sent
a snapshot with a TxnSpanGCThresholdKey but without the corresponding
value in SnapshotRequest_Header.ReplicaState.TxnSpanGCThreshold. This
commit avoids this assertion failure by continuing to send this field
in the snapshot header, even though it is no longer maintained.
19.2 nodes will ignore the field during entry application and during
snapshot ingestion, so the change has no effect on them. However,
we can rest assured that the same assertion would fire if we messed
this up on 19.2 nodes.
Release note: None