Skip to content

clusterversion,*: remove pre-22.1 cluster versions#86642

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
celiala:remove-gates.bump-min-version
Sep 1, 2022
Merged

clusterversion,*: remove pre-22.1 cluster versions#86642
craig[bot] merged 1 commit intocockroachdb:masterfrom
celiala:remove-gates.bump-min-version

Conversation

@celiala
Copy link
Copy Markdown
Collaborator

@celiala celiala commented Aug 23, 2022 •
edited
Loading

This PR removes any remaining pre-22.1 gates and bumps the minimum-supported version on master, which is required in order for us to cut any release for 22.2

The cleanup for these version gates was fairly straight-forward, using the guidance from #74270 (comment):

For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

Code removed / adjusted, by team-specific areas:

  • TestRestoreOldVersions: removed BinaryVersionOverride reference to old version (cc @RichardJCai / @cockroachdb/bulk-io )
  • Removed TestVirtualColumnNotAllowedInPkeyBefore22_1 (cc @cockroachdb/sql-schema )
  • Adjusted TestSetMinVersion to account for deprecated version gates (cc @cockroachdb/storage )

Fixes #80663
Release note: None
Release justification: bump minSupportedVersion

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 23, 2022

This change is Reviewable

@celiala celiala changed the title clusterversion,sql,backupccl: remove PublicSchemasWithDescriptors clusterversion,sql,backupccl: remove pre-22.1 cluster versions Aug 23, 2022
@celiala celiala changed the title clusterversion,sql,backupccl: remove pre-22.1 cluster versions clusterversion,*sql,backupccl*: remove pre-22.1 cluster versions Aug 23, 2022
@celiala celiala changed the title clusterversion,*sql,backupccl*: remove pre-22.1 cluster versions clusterversion,*: remove pre-22.1 cluster versions Aug 23, 2022
@celiala celiala force-pushed the remove-gates.bump-min-version branch 3 times, most recently from bfbcbf0 to dc05e1c Compare August 23, 2022 17:22
@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Aug 23, 2022 •
edited
Loading

TestRestoreOldVersions seems to be consistently failing:

  • FYI @ajwerner that I'm going to skip this test to unblock this PR, as I see this test has been skipped in the past (71389, 70155. But holler if we should take an alternate action.
  • 8/30 Update: I've left test as-is, but I'll need help to get TestRestoreOldVersions to pass

@celiala celiala force-pushed the remove-gates.bump-min-version branch from dc05e1c to 0610d7b Compare August 23, 2022 18:34
@celiala celiala marked this pull request as ready for review August 23, 2022 18:36
@celiala celiala requested review from a team as code owners August 23, 2022 18:36
@celiala celiala requested review from a team August 23, 2022 18:36
@celiala celiala requested a review from a team as a code owner August 23, 2022 18:36
@celiala celiala force-pushed the remove-gates.bump-min-version branch from 0610d7b to ffc0507 Compare August 23, 2022 20:24
@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Aug 23, 2022 •
edited
Loading

8/30 Update: TestChangefeedErrors no longer fails 🎉

TestChangefeedErrors (github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl) is failing at:
I'm not sure how to fix - maybe @RaduBerinde or @cucaroach can help here?

@celiala celiala requested review from ajwerner and cucaroach August 24, 2022 13:50
@cucaroach
Copy link
Copy Markdown
Contributor

cucaroach commented Aug 24, 2022

I'm not sure how to fix - maybe @RaduBerinde or @cucaroach can help here?

Beats me, I suspect someone from CDC will know? Looks like the enabling feature of the didn't take for some reason.

@knz
Copy link
Copy Markdown
Contributor

knz commented Aug 24, 2022

cc @stevendanna could you provide input on the above?

@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Aug 24, 2022

TestRestoreOldVersions

This is a rather important test for BulkIO. I think we need somebody from their team to sign off (@stevendanna being a good choice).

@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Aug 24, 2022 •
edited by celiala
Loading

For the changefeed tests, maybe @miretskiy can help?

8/30 Update (from celia): TestChangefeedErrors no longer fails

@celiala celiala force-pushed the remove-gates.bump-min-version branch 2 times, most recently from d7dde23 to b32ae3c Compare August 26, 2022 19:42
@celiala celiala mentioned this pull request Aug 26, 2022
Closed
2 tasks
@celiala celiala force-pushed the remove-gates.bump-min-version branch 5 times, most recently from f5f21ce to a8bf7e3 Compare August 30, 2022 18:47
@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Aug 30, 2022

Re TestRestoreOldVersions:

8/30 Update: I've updated the PR to leave the TestRestoreOldVersions test as-is, but I could definitely I could use help from someone on how to fix (maybe @stevendanna / BulkIO ) ?

The CI output is a bit vague (Lots of "No pass/skip/fail event found for test"):
https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelEssentialCi/6296779?buildTab=tests&status=failed

@celiala
clusterversion,storage,backupccl: remove pre-22.1 gates, bump minVersion
c083eb3 
Release justification: bump min supported version
Release note: None
@celiala celiala force-pushed the remove-gates.bump-min-version branch from a8bf7e3 to c083eb3 Compare September 1, 2022 12:13
@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Sep 1, 2022

Hi all - this is now RFAL - thanks!

All tests now pass for Bazel Essential CI. Here is code that was removed/tweaked, by team-specific areas:

  • TestRestoreOldVersions: removed BinaryVersionOverride reference to old version (cc @RichardJCai / @cockroachdb/bulk-io )
  • Removed TestVirtualColumnNotAllowedInPkeyBefore22_1 (cc @cockroachdb/sql-schema )
  • Adjusted TestSetMinVersion to account for deprecated version gates (cc @cockroachdb/storage )

Thanks!

@postamar
Copy link
Copy Markdown

postamar commented Sep 1, 2022

Schema-related changes LGTM

ajwerner
ajwerner approved these changes Sep 1, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Schema changes :lgtm:, @postamar won the race

Reviewed 1 of 11 files at r1, 1 of 5 files at r2, 2 of 4 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @cucaroach, and @irfansharif)

benbardin
benbardin approved these changes Sep 1, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@benbardin benbardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved for bakcupccl

@celiala celiala requested a review from jlinder September 1, 2022 17:15
jbowens
jbowens approved these changes Sep 1, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

storage related changes :lgtm:

Reviewed 2 of 11 files at r1, 3 of 5 files at r2.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @adityamaru, @cucaroach, @irfansharif, and @jlinder)

@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Sep 1, 2022

TFTRs!

bors r=jlinder,benbardin,jbowens,ajwerner

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Sep 1, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Sep 1, 2022

Build succeeded:

@craig craig bot merged commit fde7aaf into cockroachdb:master Sep 1, 2022
@erikgrinaker erikgrinaker mentioned this pull request Sep 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jlinder jlinder jlinder approved these changes

@benbardin benbardin benbardin approved these changes

@adityamaru adityamaru Awaiting requested review from adityamaru

@irfansharif irfansharif Awaiting requested review from irfansharif

@cucaroach cucaroach Awaiting requested review from cucaroach

+2 more reviewers

@jbowens jbowens jbowens approved these changes

@ajwerner ajwerner ajwerner approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

clusterversion: bump min-supported version to 22.1

9 participants

@celiala @cockroach-teamcity @cucaroach @knz @ajwerner @postamar @jlinder @benbardin @jbowens