Skip to content

clusterversion,changefeedccl: remove Changefeed version gates#85937

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
celiala:remove-gates.ChangefeedIdleness
Aug 20, 2022
Merged

clusterversion,changefeedccl: remove Changefeed version gates#85937
craig[bot] merged 2 commits intocockroachdb:masterfrom
celiala:remove-gates.ChangefeedIdleness

Conversation

@celiala
Copy link
Copy Markdown
Collaborator

@celiala celiala commented Aug 11, 2022 •
edited
Loading

This commit removes the 22.1 ChangefeedIdleness and EnableNewChangefeedOptions version gates.

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:11
@celiala celiala requested review from stevendanna and removed request for a team August 11, 2022 01:11
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 11, 2022

This change is Reviewable

@celiala celiala requested review from a team, gh-casper, samiskin and shermanCRL and removed request for a team, gh-casper and stevendanna August 11, 2022 01:11
@celiala celiala force-pushed the remove-gates.ChangefeedIdleness branch from be622ad to 17b0465 Compare August 11, 2022 22:31
@celiala celiala mentioned this pull request Aug 15, 2022
Closed
2 tasks
@celiala celiala force-pushed the remove-gates.ChangefeedIdleness branch 3 times, most recently from a0dabe6 to 346a0f5 Compare August 19, 2022 01:50
@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Aug 19, 2022 •
edited
Loading

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

This was referenced Aug 19, 2022
Merged
Merged
@celiala celiala force-pushed the remove-gates.ChangefeedIdleness branch from 346a0f5 to 5b987cc Compare August 19, 2022 07:06
@celiala celiala changed the title clusterversion,changefeedccl: remove ChangefeedIdleness clusterversion,changefeedccl: remove Changefeed version gates Aug 19, 2022
@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Aug 19, 2022

This tests fails - not sure how to fix - hopefully someone on CDC can help here?

  • pkg/ccl/logictestccl/tests/local-mixed-21.2-22.1/local-mixed-21_2-22_1_test: TestCCLLogic_create_changefeed_mixed_versions

TestCCLLogic_create_changefeed_mixed_versions output:

        expected:
        pq: option end_time is not supported until upgrade to version EnableNewChangefeedOptions or higher is finalized
        got:
        pq: specified end time 1.0000000000 cannot be less than statement time 1660894120070553817.0000000000

@celiala celiala force-pushed the remove-gates.ChangefeedIdleness branch from 5b987cc to 601ad31 Compare August 19, 2022 13:47
celiala added 2 commits August 19, 2022 19:33
@celiala @samiskin
clusterversion,changefeedccl: remove ChangefeedIdleness
6300e8f 
Release note: None
Release justification: Removing old version gate.
@celiala @samiskin
clusterversion,changefeedccl: remove EnableNewChangefeedOptions
fa49b05 
Release justification: remove old version gate.
Release note: None
@samiskin samiskin force-pushed the remove-gates.ChangefeedIdleness branch from 601ad31 to fa49b05 Compare August 19, 2022 19:37
@samiskin
Copy link
Copy Markdown
Contributor

samiskin commented Aug 19, 2022

rebased and force-pushed a version that deletes the local-mixed test. It was only being used to verify we check against these options that no longer exist to be checked.

samiskin
samiskin approved these changes Aug 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@samiskin samiskin left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Aug 19, 2022

TFTR and help with the local-mixed test! 🙌🏼

bors r=samiskin

@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 20, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 20, 2022

Build succeeded:

@craig craig bot merged commit cb55144 into cockroachdb:master Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shermanCRL shermanCRL Awaiting requested review from shermanCRL shermanCRL is a code owner automatically assigned from cockroachdb/cdc-prs

1 more reviewer

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