-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clusterversion: bump min supported version to 23.1 #112122
Conversation
35628eb
to
5e6f619
Compare
5e6f619
to
25c8d43
Compare
5903dc6
to
5ee9b6c
Compare
5ee9b6c
to
abb6a59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this HUGE PR!
Reviewed 130 of 130 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @DarrylWong, @dt, @herkolategan, @jbowens, and @mgartner)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only looked at the pkg/storage changes, but they look good to me.
Reviewed 3 of 130 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @DarrylWong, @dt, @herkolategan, and @mgartner)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 130 of 130 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @celiala, @DarrylWong, @dt, @herkolategan, @mgartner, and @RaduBerinde)
pkg/sql/logictest/testdata/logic_test/drop_owned_by
line 648 at r2 (raw file):
ALTER FUNCTION f_drop_udf OWNER TO u_drop_udf; # TODO(chengxiong): DROP function was only enabled in 23.1. This skip can be
nit: a couple of TODOs should now be removed.
pkg/sql/logictest/testdata/logic_test/gc_job_mixed
line 23 at r2 (raw file):
DROP DATABASE db # GC jobs in a non-mixed-version cluster, 23.1+ style.
nit: perhaps remove this comment too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Reviewed at a pretty-high level - changes LGTM
Reviewed 130 of 130 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @dt, @herkolategan, @mgartner, and @RaduBerinde)
pkg/clusterversion/cockroach_versions.go
line 536 at r2 (raw file):
// version of the release in development). Tests should use this constant so // they don't need to be updated when the versions change. const VCurrent_Start = V23_2Start
+1 to the practice of using VCurrent_Start
as an alias for the last Start version key!
I'm wondering how best to communicate this out?
(i.e. will the team know to use VCurrent_Start
, or will they continue using V23_2Start
because they don't know about this..).
pkg/sql/tenant_creation.go
line 50 at r2 (raw file):
const ( tenantCreationMinSupportedVersionKey = clusterversion.BinaryMinSupportedVersionKey
(I don't know enough about how tenantCreationMinSupportedVersionKey
differs from BinaryMinSupportedVersionKey
)
but I'm guessing someone on the multi-tenant team has / can weigh-in on this?
pkg/sql/logictest/logictestbase/logictestbase.go
line 270 at r2 (raw file):
// See DefaultConfigNames for the list of default configs. // // Note: If you add a new config, you should run `./dev gen testlogic`.
(thank you for adding this note!)x
pkg/ccl/serverccl/server_startup_guardrails_test.go
line 65 at r2 (raw file):
// version is not too low for the tenant logical version. { storageBinaryVersion: logicalVersion,
nice! thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!!
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @celiala, @DarrylWong, @dt, @herkolategan, and @mgartner)
pkg/clusterversion/cockroach_versions.go
line 536 at r2 (raw file):
Previously, celiala wrote…
+1 to the practice of using
VCurrent_Start
as an alias for the last Start version key!I'm wondering how best to communicate this out?
(i.e. will the team know to useVCurrent_Start
, or will they continue usingV23_2Start
because they don't know about this..).
I plan to consider this when doing more work on this stuff on master - one option is to unexport V23_2Start
and similar if they have no uses that can't be better served by VCurrent_Start
; another is a linter, or just a comment above every Start
key explaining VCurrent
should be preferred.
pkg/sql/tenant_creation.go
line 50 at r2 (raw file):
Previously, celiala wrote…
(I don't know enough about how
tenantCreationMinSupportedVersionKey
differs fromBinaryMinSupportedVersionKey
)but I'm guessing someone on the multi-tenant team has / can weigh-in on this?
I had asked on Slack, this is the discussion https://cockroachlabs.slack.com/archives/C02J7BTSRK2/p1696979372260889
This commit changes the minimum supported version to 23.1. It is intended to be backported to release-23.2. Tests that had to be modified mostly fall in one of two categories: - tests that verify a release-specific behavior involving 22.2 or in-development versions of 23.1; these tests were deleted. - tests that verify general version handling logic but used specific versions; I tried to make these use the min supported version or a new `VCurrent_Start` constant. The logic tests required a bunch of work because we were missing a `local-mixed-23.1` config, which should have been added early in the 23.2 cycle. Without this config, all test directives related to 23.2 features were conditioned on `local-mixed-22-23.1` (i.e. these features were lumped in with 23.1 features). I added the new config and let the test failures guide me; most of the cases that needed to be conditioned on `local-mixed-23.1` where around the bigger 23.2 features (isolation levels, procedures). In subsequent release cycles, we will create the new config as soon as possible. This commit does not remove the obsolete in-development 23.1 version keys and related code; that will be done separately. Informs: cockroachdb#111760 Epic: REL-506 Release note: None
abb6a59
to
8c028a9
Compare
bors r+ |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 8c028a9 to blathers/backport-release-23.2-112122: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This commit changes the minimum supported version to 23.1. It is
intended to be backported to release-23.2.
Tests that had to be modified mostly fall in one of two categories:
in-development versions of 23.1; these tests were deleted.
versions; I tried to make these use the min supported version or a
new
VCurrent_Start
constant.The logic tests required a bunch of work because we were missing a
local-mixed-23.1
config, which should have been added early in the23.2 cycle. Without this config, all test directives related to 23.2
features were conditioned on
local-mixed-22.2-23.1
(i.e. lumped inwith 23.1 features). I added the new config and let the test failures
guide me; most of the cases that needed to be conditioned on
local-mixed-23.1
where around the bigger 23.2 features (isolationlevels, procedures). In subsequent release cycles, we will create the
new config as soon as possible.
This commit does not remove the obsolete in-development 23.1 version
keys and related code; that will be done separately.
Informs: #111760
Epic: REL-506
Release note: None