storage: remove pre-22.2 code#96805
Conversation
|
This change is |
|
The only material change is adding a new test case to |
|
900+ failed tests haha, obviously this needs more work. Making this a draft. |
cdabc01 to
665355c
Compare
|
Ready for review. |
sumeerbhola
left a comment
There was a problem hiding this comment.
And is this safe to do because we dropped the need for 23.1 binaries to handle a transition from 22.1 due to the postponement outlined in https://groups.google.com/a/cockroachlabs.com/g/eng/c/v4hgN-rfEnk/m/VgDuolKeCQAJ?utm_medium=email&utm_source=footer?
Thanks for doing this cleanup!
Reviewed 14 of 19 files at r1, 6 of 6 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)
pkg/storage/pebble.go line 1513 at r3 (raw file):
func shouldWriteLocalTimestamps(ctx context.Context, settings *cluster.Settings) bool { if !LocalTimestampsEnabled.Get(&settings.SV) {
let's confirm with @nvanbenschoten that there hasn't been some new thinking regarding local timestamps.
pkg/sql/gcjob_test/gc_job_test.go line 672 at r3 (raw file):
// initialization. // // TODO(ajwerner): Remove this test in 23.1.
can you confirm with @ajwerner
pkg/upgrade/upgrademanager/manager_external_test.go line 305 at r3 (raw file):
// We're going to be migrating from startKey to endKey. We end up needing // to use real versions because the ListBetween uses the keys compiled into
why was is saying "real versions" when startMajor was 42 and our current real highest value for Major is 22?
RaduBerinde
left a comment
There was a problem hiding this comment.
TFTR!
Yes, the binary min version has been bumped to v22.2, so transitioning from 22.1 is off the table.
Reviewable status:
pkg/upgrade/upgrademanager/manager_external_test.go line 305 at r3 (raw file):
Previously, sumeerbhola wrote…
why was is saying "real versions" when
startMajorwas 42 and our current real highest value forMajoris 22?
Feels like the comment doesn't match the code (anymore?). In any case, I needed this test change in an earlier variant where I didn't allow older versions in SetMinVersion. But I left it because this is cleaner.
951c59d to
06edc6a
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/pebble.go line 1513 at r3 (raw file):
Previously, sumeerbhola wrote…
let's confirm with @nvanbenschoten that there hasn't been some new thinking regarding local timestamps.
Keeping the cluster setting but removing the version check SGTM.
This change removes any code that handles pre-22.2 versions. Features that are in 22.2 (like range keys) are now always enabled. Any new store always uses at least the 22.2 pebble format version. Release note: None Epic: none Fixes: cockroachdb#96764
06edc6a to
12aec72
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/gcjob_test/gc_job_test.go line 672 at r3 (raw file):
Previously, sumeerbhola wrote…
can you confirm with @ajwerner
He is out for a couple of days, I will merge this for now and double-check with him afterward.
|
bors r+ |
|
Build succeeded: |
|
I think this PR made some of the benchmarks fail. On 8511dc0 |
|
Thanks, yes, #97066 will hopefully fix that. |
This change removes any code that handles pre-22.2 versions. Features
that are in 22.2 (like range keys) are now always enabled.
Any new store always uses at least the 22.2 pebble format version.
Release note: None
Epic: none
Fixes: #96764