clusterversion: bump min supported version to 23.1#112122
clusterversion: bump min supported version to 23.1#112122craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
35628eb to
5e6f619
Compare
5e6f619 to
25c8d43
Compare
5903dc6 to
5ee9b6c
Compare
5ee9b6c to
abb6a59
Compare
rail
left a comment
There was a problem hiding this comment.
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)
jbowens
left a comment
There was a problem hiding this comment.
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:
yuzefovich
left a comment
There was a problem hiding this comment.
Thanks!
Reviewed 130 of 130 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
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?
celiala
left a comment
There was a problem hiding this comment.
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:
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!
RaduBerinde
left a comment
There was a problem hiding this comment.
TFTR!!
Reviewable status:
pkg/clusterversion/cockroach_versions.go line 536 at r2 (raw file):
Previously, celiala wrote…
+1 to the practice of using
VCurrent_Startas 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_2Startbecause 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
tenantCreationMinSupportedVersionKeydiffers 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_Startconstant.The logic tests required a bunch of work because we were missing a
local-mixed-23.1config, 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.1where 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