Skip to content
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,*: remove pre-22.1 cluster versions #86642

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

celiala
Copy link
Collaborator

@celiala celiala commented Aug 23, 2022

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
Member

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
Collaborator Author

celiala commented Aug 23, 2022

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
Collaborator Author

celiala commented Aug 23, 2022

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
Contributor

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
Contributor

knz commented Aug 24, 2022

cc @stevendanna could you provide input on the above?

@ajwerner
Copy link
Contributor

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
Contributor

ajwerner commented Aug 24, 2022

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 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
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

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
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
Contributor

postamar commented Sep 1, 2022

Schema-related changes LGTM

Copy link
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)

Copy link
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
Copy link
Collaborator

@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
Collaborator Author

celiala commented Sep 1, 2022

TFTRs!

bors r=jlinder,benbardin,jbowens,ajwerner

@craig
Copy link
Contributor

craig bot commented Sep 1, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 1, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clusterversion: bump min-supported version to 22.1
9 participants