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,kvserver,sql: remove AutoStatsTableSettings #85939

Merged

Conversation

celiala
Copy link
Collaborator

@celiala celiala commented Aug 11, 2022

This commit removes the 22.1 AutoStatsTableSettings version gate.

Cleanup was done following guidance from 21.2 cleanup:

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]).

However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release note: None
Release justification: remove old version gates.

@celiala celiala requested a review from a team as a code owner August 11, 2022 01:26
@celiala celiala requested a review from a team August 11, 2022 01:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@celiala celiala requested a review from msirek August 11, 2022 01:27
Copy link
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @celiala)

@celiala celiala force-pushed the remove-gates.AutoStatsTableSettings branch from b752c83 to a2a26ea Compare August 11, 2022 22:40
@celiala
Copy link
Collaborator Author

celiala commented Aug 11, 2022

TFTR!

FYI @msirek that I'm removing TestAutoStatsTableSettingsDisallowedOnOldCluster -- please scream and shout if this should be kept!

Removing TestAutoStatsTableSettingsDisallowedOnOldCluster since:

  • it was failing (expected "feature not available yet" error but instead got nil)
  • it depended on both ClusterLocksVirtualTable and AutoStatsTableSettings -- both of which are 22.1 version gates

@celiala celiala force-pushed the remove-gates.AutoStatsTableSettings branch from a2a26ea to 321e34e Compare August 12, 2022 03:10
Copy link
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

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

It should be OK. This change will only be in 22.2. My understanding is, upgrades are only allowed one minor release at a time, so any nodes running an old version of CRDB would be on 22.1 at the oldest, which does support AutoStatsTableSettings.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @msirek)

@celiala
Copy link
Collaborator Author

celiala commented Aug 19, 2022

TestLogic_external_connection_privileges is failing all of #85937 , #85938 , #85939, so I'm going to assume that it's not related to changes in this commit.

off_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/external_connection_privileges:24: SELECT * FROM system.privileges
        expected:
            root      /externalconn/foo  {ALL}         {}
            testuser  /externalconn/foo  {DROP,USAGE}  {}
        but found (query options: "") :
            testuser  /externalconn/foo  {DROP,USAGE}  {}
            root      /externalconn/foo  {ALL}         {}

@celiala celiala force-pushed the remove-gates.AutoStatsTableSettings branch from 26da725 to 9c1b564 Compare August 19, 2022 07:24
Release note: None
Release justification: Remove old version gate.
@celiala celiala force-pushed the remove-gates.AutoStatsTableSettings branch from 9c1b564 to 6697549 Compare August 19, 2022 13:54
@celiala
Copy link
Collaborator Author

celiala commented Aug 19, 2022

TFTR!

bors r=msirek

@craig
Copy link
Contributor

craig bot commented Aug 19, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 19, 2022

Build succeeded:

@craig craig bot merged commit 677ef2c into cockroachdb:master Aug 19, 2022
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.

3 participants