clusterversion: cockroach versions cleanup #113314

RaduBerinde · 2023-10-30T19:57:54Z

This commit cleans up the code and constants in
cockroach_versions.go:

we separate out the "dev offset" code and improve the documentation;
we replace the key-to-version slice with a table (array). This makes the declaration much cleaner and makes the lookup easier.
we replace ByKey() with a method, to make the call site less messy, e.g. clusterversion.ByKey(clusterversion.V23_2) becomes clusterversion.V23_2.Version();
we deprecate BinaryMinSupportedVersionKey in favor of MinSupported;
we deprecate BinaryVersionKey in favor of Latest;
we remove roachpb.Version "constants" in favor of using the Version() method.

Informs: #112629
Release note: None

blathers-crl · 2023-10-30T19:57:57Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-10-30T19:58:03Z

This change is

RaduBerinde · 2023-10-30T21:30:49Z

I plan to do similar related cleanup (e.g. to the methods of the version Handle) once this is approved.

RaduBerinde · 2023-11-01T14:28:15Z

Friendly ping

andyyang890

Started reviewing this and it looks pretty good so far! Had a few questions and a small nit.

Reviewed 4 of 15 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi, @celiala, @dt, @RaduBerinde, and @rail)

pkg/clusterversion/cockroach_versions.go line 517 at r1 (raw file):

//
// Note that versions in this table can be modified by applying a "dev offset"
// to ensure that upgrades don't occur between in-developent and relased

nit: s/developent/development/ and s/relased/released/

pkg/clusterversion/dev_offset.go line 41 at r1 (raw file):

// version! For example, on a cluster that is on released version 3, a dev
// binary containing versions 1, 2, 3, and 4 started with this flag would
// renumber only 2-4 to be +1M. It would then step from 3 "up" to 1000002 --

// ... For example, on a cluster that is on released version 3, a dev
// binary containing versions 1, 2, 3, and 4 started with this flag would
// renumber only 2-4 to be +1M. ...

I know this comment was just copied over but I'm kind of confused about this sentence. Is it because we had the implicit assumption before that MinSupported is 1 version lower (i.e. 2 in this example)?

pkg/clusterversion/dev_offset.go line 46 at r1 (raw file):

var devOffsetKeyStart = func() Key {
	forceDev := envutil.EnvOrDefaultBool("COCKROACH_FORCE_DEV_VERSION", false)
	forceRelease := envutil.EnvOrDefaultBool("COCKROACH_TESTING_FORCE_RELEASE_BRANCH", false)

What is the use case for COCKROACH_TESTING_FORCE_RELEASE_BRANCH? It seems like it would be used to allow us to build a non-dev binary from a release branch after it is cut and before we change developmentBranch to false. When would we want to do this?

Also, do we want to rename it to something like COCKROACH_FORCE_NON_DEV_VERSION or something more specific that indicates what exactly we are forcing?

RaduBerinde

Thanks for reviewing!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi, @andyyang890, @celiala, @dt, and @rail)

pkg/clusterversion/cockroach_versions.go line 517 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

nit: s/developent/development/ and s/relased/released/

Fixed.

pkg/clusterversion/dev_offset.go line 41 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

// ... For example, on a cluster that is on released version 3, a dev
// binary containing versions 1, 2, 3, and 4 started with this flag would
// renumber only 2-4 to be +1M. ...
I know this comment was just copied over but I'm kind of confused about this sentence. Is it because we had the implicit assumption before that MinSupported is 1 version lower (i.e. 2 in this example)?

Correct, I improved the comment.

pkg/clusterversion/dev_offset.go line 46 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

What is the use case for COCKROACH_TESTING_FORCE_RELEASE_BRANCH? It seems like it would be used to allow us to build a non-dev binary from a release branch after it is cut and before we change developmentBranch to false. When would we want to do this?

Also, do we want to rename it to something like COCKROACH_FORCE_NON_DEV_VERSION or something more specific that indicates what exactly we are forcing?

It is used for some tests where we actually want to test things as they would happen in the "real" release (as much as possible).

I agree the name could be better but I'm not keen on renaming it in case it is already in use (e.g. in scripts).

celiala

O.M.G. - this is such an elegant-looking improvement 😍 -- thank you for doing this!
LGTM!

Reviewed 13 of 15 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi, @andyyang890, @dt, @RaduBerinde, and @rail)

pkg/clusterversion/cockroach_versions.go line 622 at r2 (raw file):

// Note: this is always the last element of SupportedPreviousReleases(); it is
// also provided as a constant for convenience.
const PreviousRelease Key = V23_2

maybe linter to verify that PreviousRelease always equals Latest - 1?

pkg/clusterversion/cockroach_versions.go line 633 at r2 (raw file):

// DEPRECATED: use MinSupported.
const BinaryMinSupportedVersionKey = MinSupported

should we rename BinaryMinSupportedVersionKey to 'DEPRECATED_BinaryMinSupportedVersionKey`? (suggestion for future, not this PR necessarily)

pkg/clusterversion/cockroach_versions.go line 635 at r2 (raw file):

const BinaryMinSupportedVersionKey = MinSupported

// DEPRECATED: use Latest.

(same)

pkg/clusterversion/cockroach_versions.go line 661 at r2 (raw file):

			panic("latest version does not match final version")
		}
		if developmentBranch {

add the "if Previous != Latest - 1 then panic" check here?

RaduBerinde

Thanks!!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi, @andyyang890, @celiala, @dt, and @rail)

pkg/clusterversion/cockroach_versions.go line 622 at r2 (raw file):

Previously, celiala wrote…

maybe linter to verify that PreviousRelease always equals Latest - 1?

Note that it is not "previous version" but "previous release". So it's the last version key before Latest that has Internal = 0.TestKeyConstants verifies the constant.

pkg/clusterversion/cockroach_versions.go line 633 at r2 (raw file):

Previously, celiala wrote…

should we rename BinaryMinSupportedVersionKey to 'DEPRECATED_BinaryMinSupportedVersionKey`? (suggestion for future, not this PR necessarily)

Just adding the comment is enough to make all uses look crossed out in Goland. I plan to clean it up soon, just didn't want to make it all in a huge PR.

andyyang890

This is awesome!

Reviewed 10 of 15 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aadityasondhi, @celiala, @dt, @RaduBerinde, and @rail)

pkg/clusterversion/dev_offset.go line 46 at r1 (raw file):

Previously, RaduBerinde wrote…

It is used for some tests where we actually want to test things as they would happen in the "real" release (as much as possible).

I agree the name could be better but I'm not keen on renaming it in case it is already in use (e.g. in scripts).

Ah got it, would it be possible to maybe add a note documenting it to the comment above devOffsetKeyStart?

RaduBerinde

TFTRs!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aadityasondhi, @andyyang890, @celiala, @dt, and @rail)

pkg/clusterversion/dev_offset.go line 46 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Ah got it, would it be possible to maybe add a note documenting it to the comment above devOffsetKeyStart?

Done. Good idea, I added some documentation for both env vars.

craig · 2023-11-02T21:44:08Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

renatolabs · 2023-11-02T21:47:16Z

pkg/clusterversion/setting_test.go:69:51: undefined: clusterversion.VCurrent_Start

bors r-

craig · 2023-11-02T21:47:19Z

Canceled.

This commit cleans up the code and constants in `cockroach_versions.go`: - we separate out the "dev offset" code and improve the documentation; - we replace the key-to-version slice with a table (array). This makes the declaration much cleaner and makes the lookup easier. - we replace `ByKey()` with a method, to make the call site less messy; e.g. `clusterversion.ByKey(clusterversion.V23_2)` becomes `clusterversion.V23_2.Version()`; - we deprecate `BinaryMinSupportedVersionKey` in favor of `MinSupported`; - we deprecate `BinaryVersionKey` in favor of `Latest`; - we remove `roachpb.Version` "constants" in favor of using the `Version()` method. Informs: cockroachdb#112629 Release note: None

RaduBerinde · 2023-11-03T03:23:57Z

bors r+

craig · 2023-11-03T04:06:40Z

Build succeeded:

Bazel Essential CI (Cockroach)

RaduBerinde force-pushed the version-cleanup branch from 93d6bfb to 371eec9 Compare October 30, 2023 20:35

RaduBerinde requested review from dt, rail and celiala October 30, 2023 20:35

RaduBerinde marked this pull request as ready for review October 30, 2023 20:36

RaduBerinde requested a review from a team as a code owner October 30, 2023 20:36

RaduBerinde force-pushed the version-cleanup branch from 371eec9 to 5cce778 Compare October 30, 2023 21:20

RaduBerinde requested a review from a team as a code owner October 30, 2023 21:20

RaduBerinde requested a review from aadityasondhi October 30, 2023 21:20

RaduBerinde requested a review from andyyang890 November 2, 2023 17:33

andyyang890 reviewed Nov 2, 2023

View reviewed changes

RaduBerinde force-pushed the version-cleanup branch from 5cce778 to f3964a0 Compare November 2, 2023 19:19

RaduBerinde commented Nov 2, 2023

View reviewed changes

celiala requested a review from andyyang890 November 2, 2023 19:50

celiala approved these changes Nov 2, 2023

View reviewed changes

RaduBerinde commented Nov 2, 2023

View reviewed changes

andyyang890 approved these changes Nov 2, 2023

View reviewed changes

RaduBerinde force-pushed the version-cleanup branch 2 times, most recently from 728991b to 3b67dcb Compare November 2, 2023 21:26

RaduBerinde commented Nov 2, 2023

View reviewed changes

RaduBerinde force-pushed the version-cleanup branch from 3b67dcb to 9f4a5d8 Compare November 3, 2023 00:43

craig bot merged commit 694861a into cockroachdb:master Nov 3, 2023
3 checks passed

RaduBerinde deleted the version-cleanup branch November 3, 2023 13:55

andyyang890 mentioned this pull request Nov 3, 2023

clusterversion: export binary{,MinSupported}Version #111549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clusterversion: cockroach versions cleanup #113314

clusterversion: cockroach versions cleanup #113314

RaduBerinde commented Oct 30, 2023 •

edited

Loading

blathers-crl bot commented Oct 30, 2023

cockroach-teamcity commented Oct 30, 2023

RaduBerinde commented Oct 30, 2023

RaduBerinde commented Nov 1, 2023

andyyang890 left a comment

RaduBerinde left a comment

celiala left a comment

RaduBerinde left a comment

andyyang890 left a comment

RaduBerinde left a comment

craig bot commented Nov 2, 2023

renatolabs commented Nov 2, 2023

craig bot commented Nov 2, 2023

RaduBerinde commented Nov 3, 2023

craig bot commented Nov 3, 2023

clusterversion: cockroach versions cleanup #113314

clusterversion: cockroach versions cleanup #113314

Conversation

RaduBerinde commented Oct 30, 2023 • edited Loading

blathers-crl bot commented Oct 30, 2023

cockroach-teamcity commented Oct 30, 2023

RaduBerinde commented Oct 30, 2023

RaduBerinde commented Nov 1, 2023

andyyang890 left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

celiala left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

andyyang890 left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

craig bot commented Nov 2, 2023

renatolabs commented Nov 2, 2023

craig bot commented Nov 2, 2023

RaduBerinde commented Nov 3, 2023

craig bot commented Nov 3, 2023

RaduBerinde commented Oct 30, 2023 •

edited

Loading