-
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: create 24.3 version #128616
Conversation
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. |
84f9536
to
82e0ba0
Compare
724e234
to
85cbea2
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @RaduBerinde, and @renatolabs)
pkg/clusterversion/cockroach_versions.go
line 334 at r1 (raw file):
V24_2_TenantRates: {Major: 24, Minor: 1, Internal: 8}, V24_2_DeleteTenantSettingsVersion: {Major: 24, Minor: 1, Internal: 10}, V24_2_LeaseMinTimestamp: {Major: 24, Minor: 1, Internal: 12},
I don't think we should change V24_2_LeaseMinTimestamp
. I only piggybacked off it in #128647. It was introduced before and is used elsewhere. It exists on release-24.2
, so I imagine making this change would cause issues.
Instead, we should leave this as is, and introduce a new V24_3_StoreLivenessEnabled
version after V24_3Start
. Then, if we use this in replica_store_liveness.go, I think all tests should pass. I had some words around this in this TODO:
cockroach/pkg/kv/kvserver/replica_store_liveness.go
Lines 75 to 78 in 167047e
// liveness is enabled. The other incorrect bit about this version check is | |
// that it re-uses clusterversion.V24_2_LeaseMinTimestamp; we should instead | |
// introduce a new version (V24_3_StoreLivenessEnabled); this can only happen | |
// once https://github.com/cockroachdb/cockroach/pull/128616 lands. |
85cbea2
to
b70eb66
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @celiala, and @renatolabs)
pkg/clusterversion/cockroach_versions.go
line 334 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I don't think we should change
V24_2_LeaseMinTimestamp
. I only piggybacked off it in #128647. It was introduced before and is used elsewhere. It exists onrelease-24.2
, so I imagine making this change would cause issues.Instead, we should leave this as is, and introduce a new
V24_3_StoreLivenessEnabled
version afterV24_3Start
. Then, if we use this in replica_store_liveness.go, I think all tests should pass. I had some words around this in this TODO:
cockroach/pkg/kv/kvserver/replica_store_liveness.go
Lines 75 to 78 in 167047e
// liveness is enabled. The other incorrect bit about this version check is // that it re-uses clusterversion.V24_2_LeaseMinTimestamp; we should instead // introduce a new version (V24_3_StoreLivenessEnabled); this can only happen // once https://github.com/cockroachdb/cockroach/pull/128616 lands.
Ohh I see now. I had thought I looked at the release-24.2
branch and didn't see the gate but I must have messed something up. This makes a lot more sense now. Done. Thanks!!
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 2 of 10 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @celiala, @RaduBerinde, and @renatolabs)
pkg/clusterversion/cockroach_versions.go
line 334 at r1 (raw file):
Previously, RaduBerinde wrote…
Ohh I see now. I had thought I looked at the
release-24.2
branch and didn't see the gate but I must have messed something up. This makes a lot more sense now. Done. Thanks!!
Thanks, and sorry for the trouble my hack to merge that PR caused you.
pkg/clusterversion/cockroach_versions.go
line 278 at r2 (raw file):
V24_3_Start // V24_3_StoreLivenessEnabled TODO
nit: V24_3_StoreLivenessEnabled is the earliest version which supports the use of the StoreLiveness fabric.
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! LGTM
(modulo reverting V24_2_LeaseMinTimestamp back to original value, as discussed)
Reviewed 43 of 49 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @renatolabs)
pkg/clusterversion/cockroach_versions.go
line 346 at r2 (raw file):
V24_2_LeaseMinTimestamp: {Major: 24, Minor: 2, Internal: 12}, V24_2: {Major: 24, Minor: 2, Internal: 0},
oops. I added this to release-24.2 but forgot to also add this to master. sorry, my bad (noted in my runbook for next time).x//.x.
Previously, celiala wrote…
No, it wouldn't have worked - we can't have an internal=0 as the latest version of master (a dev branch). I think it always needs to be done together with starting the new release versions. |
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @celiala, and @renatolabs)
pkg/clusterversion/cockroach_versions.go
line 278 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: V24_3_StoreLivenessEnabled is the earliest version which supports the use of the StoreLiveness fabric.
Oops, thanks, done!
b70eb66
to
479591d
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.
LGTM!
479591d
to
7427786
Compare
TFTRs! bors r+ |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed: |
- [x] Add version key constant for new release (e.g. `V24.2`), equal to `Latest` - [x] Update `PreviousRelease` constant - [x] Add start version (e.g. `V24.2Start` with version `24.1-2`) - [x] Update `pkg/build/version.txt` to the new version (e.g. `v24.2.0-alpha.00000000`) - [x] Add mixed version logictest config for the replaced version - [x] Update the `scplan` rules in `pkg/sql/schemachanger/scplan/internal/rules` - [x] Create new roachtest fixtures for the previous version - [x] Create new SQL bootstrap data - [x] Update releases file This change does not update the version skipping logic; in a follow-up change we will enable direct 24.1->24.3 upgrade without a special env flag. Epic: REL-1163 Release note: None
7427786
to
9e03322
Compare
bors r+ |
Correction: this was REL-1045 |
V24.2
), equal toLatest
PreviousRelease
constantV24.2Start
with version24.1-2
)pkg/build/version.txt
to the new version (e.g.v24.2.0-alpha.00000000
)scplan
rules inpkg/sql/schemachanger/scplan/internal/rules
Epic: REL-1163
Release note: None
Fixes #128747