storage: remove unused TxnSpanGCThreshold key #38817
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
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/remTxnSpanGC
branch
from
1fcd522 to
c40c8ac
Compare
tbg
approved these changes
Jul 12, 2019
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 24 of 24 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/storage/replica_test.go, line 3390 at r2 (raw file):
func TestTxnRecordUnderTxnSpanGCThreshold(t *testing.T) { defer leaktest.AfterTest(t)() t.Skip("WIP")
Reminder
pkg/storage/replica_test.go, line 7862 at r2 (raw file):
tc.Start(t, stopper) for _, keyThresh := range []hlc.Timestamp{{}, hlc.Timestamp{}.Add(1, 0)} {
useless nit, but you can write {Logical:1} here for the second element in the slice.
pkg/storage/store_test.go, line 2890 at r2 (raw file):
} clusterVersion := s.ClusterSettings().Version.Version().Version
Cue snarky comment by @andreimatei
nvanbenschoten
force-pushed
the
nvanbenschoten/remTxnSpanGC
branch
from
c40c8ac to
0a9f2e0
Compare
nvanbenschoten
commented
Jul 12, 2019
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/storage/replica_test.go, line 3390 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Reminder
Done.
pkg/storage/replica_test.go, line 7862 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
useless nit, but you can write
{Logical:1}here for the second element in the slice.
Done.
pkg/storage/store_test.go, line 2890 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Cue snarky comment by @andreimatei
Done.
Merge conflict |
…TOO_OLD This was never used in v19.1, so we can remove it now. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/remTxnSpanGC
branch
from
0a9f2e0 to
20b41f5
Compare
This was never used in v19.1, so we can remove it now. It was replaced by using to the timestamp cache to provide the same protection against GCed transaction records being re-written. See 1bf315d. The corresponding key is still replicated, so we have to be a little careful about continuing to keep it consistent across replicas. Luckily, this doesn't require too much care because any changes to the key are coordinated above Raft. v19.1 nodes will continue to include updates to the key in replicated write batches, which v19.2 nodes will unknowingly apply to update their durable state. v19.2 nodes will stop updating the key entirely and will never include changes to it in replicated write batches or ReplicatedEvalResults. The key was left in a few places, including in `pkg/keys`. This serves both as a placeholder to ensure that the key prefix is never re-used and allows us to continue to debug clusters with old TxnSpanGCThreshold keys lying around. The decision was made not to add a migration to actually delete the key on old clusters. It takes up a negligible amount of space and deleting it is not necessary for the migration. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/remTxnSpanGC
branch
from
20b41f5 to
eeecc92
Compare
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jul 12, 2019
38817: storage: remove unused TxnSpanGCThreshold key r=nvanbenschoten a=nvanbenschoten This was never used in v19.1, so we can remove it now. It was replaced by using to the timestamp cache to provide the same protection against GCed transaction records being re-written. See 1bf315d. The corresponding key is still replicated, so we have to be a little careful about continuing to keep it consistent across replicas. Luckily, this doesn't require too much care because any changes to the key are coordinated above Raft. v19.1 nodes will continue to include updates to the key in replicated write batches, which v19.2 nodes will unknowingly apply to update their durable state. v19.2 nodes will stop updating the key entirely and will never include changes to it in replicated write batches or ReplicatedEvalResults. The key was left in a few places, including in `pkg/keys`. This serves both as a placeholder to ensure that the key prefix is never re-used and allows us to continue to debug clusters with old TxnSpanGCThreshold keys lying around. The decision was made not to add a migration to actually delete the key on old clusters. It takes up a negligible amount of space and deleting it is not necessary for the migration. Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
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.
This was never used in v19.1, so we can remove it now. It was replaced by using to the timestamp cache to provide the same protection against GCed transaction records being re-written. See 1bf315d.
The corresponding key is still replicated, so we have to be a little careful about continuing to keep it consistent across replicas. Luckily, this doesn't require too much care because any changes to the key are coordinated above Raft. v19.1 nodes will continue to include updates to the key in replicated write batches, which v19.2 nodes will unknowingly apply to update their durable state. v19.2 nodes will stop updating the key entirely and will never include changes to it in replicated write batches or ReplicatedEvalResults.
The key was left in a few places, including in
pkg/keys. This serves both as a placeholder to ensure that the key prefix is never re-used and allows us to continue to debug clusters with old TxnSpanGCThreshold keys lying around.The decision was made not to add a migration to actually delete the key on old clusters. It takes up a negligible amount of space and deleting it is not necessary for the migration.