Skip to content

clusterversion,kvserver,sql: remove AutoStatsTableSettings#85939

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
celiala:remove-gates.AutoStatsTableSettings
Aug 19, 2022
Merged

clusterversion,kvserver,sql: remove AutoStatsTableSettings#85939
craig[bot] merged 1 commit intocockroachdb:masterfrom
celiala:remove-gates.AutoStatsTableSettings

Conversation

@celiala
Copy link
Copy Markdown
Collaborator

@celiala celiala commented Aug 11, 2022 •
edited
Loading

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
Copy Markdown
Member

cockroach-teamcity commented Aug 11, 2022

This change is Reviewable

@celiala celiala requested a review from msirek August 11, 2022 01:27
msirek
msirek approved these changes Aug 11, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
msirek
msirek reviewed Aug 12, 2022
View reviewed changes
Copy link
Copy Markdown
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 celiala mentioned this pull request Aug 15, 2022
Closed
2 tasks
@celiala celiala force-pushed the remove-gates.AutoStatsTableSettings branch 2 times, most recently from 71166f8 to 8aebb6b Compare August 19, 2022 01:50
This was referenced Aug 19, 2022
Merged
Merged
@celiala
Copy link
Copy Markdown
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
@celiala
clusterversion,kvserver,sql: remove AutoStatsTableSettings
6697549 
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
Copy Markdown
Collaborator Author

celiala commented Aug 19, 2022

TFTR!

bors r=msirek

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 19, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
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

Reviewers

1 more reviewer

@msirek msirek msirek 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.

3 participants

@celiala @cockroach-teamcity @msirek