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,sql,backupccl: remove PublicSchemasWithDescriptors #85825

Merged

Conversation

celiala
Copy link
Collaborator

@celiala celiala commented Aug 9, 2022

This commit removes the 22.1 version gate PublicSchemasWithDescriptors, which was referenced in:

  • pkg/ccl/backupccl/
  • pkg/ccl/changefeedccl/
  • pkg/config/zonepb/zone.go
  • pkg/sql/catalog/descs/
  • pkg/sql/descriptor.go
  • pkg/sql/importer/
  • pkg/sql/rename_table.go
  • pkg/sql/reparent_database.go

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@celiala celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from c3050df to 8d1bdb4 Compare August 9, 2022 17:43
@celiala celiala marked this pull request as ready for review August 9, 2022 17:53
@celiala celiala requested review from a team as code owners August 9, 2022 17:53
@celiala celiala requested a review from a team August 9, 2022 17:53
@celiala celiala requested review from a team as code owners August 9, 2022 17:53
@celiala celiala requested review from a team, msbutler, gh-casper, RichardJCai and ajwerner and removed request for a team August 9, 2022 17:53
@postamar postamar removed request for a team, msbutler, gh-casper and ajwerner August 10, 2022 14:24
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this. @RichardJCai can you take a look also if you have time, please ? In particular, should anything be done to the backup tests or is it safe to outright remove these such as has been done? I think it's OK, but still.

pkg/sql/rename_table.go Outdated Show resolved Hide resolved
pkg/sql/importer/import_job.go Outdated Show resolved Hide resolved
Copy link
Contributor

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

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

Only thing from me is I'd like to keep the cleanup on fail backup test. Other than that LGTM

Reviewed 17 of 19 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @postamar, and @RichardJCai)


pkg/ccl/backupccl/restore_old_versions_test.go line 1104 at r1 (raw file):

						Server: &server.TestingKnobs{
							DisableAutomaticVersionUpgrade: make(chan struct{}),
							BinaryVersionOverride:          clusterversion.ByKey(clusterversion.PublicSchemasWithDescriptors - 1),

I'd like to keep this test, I think other ones can go, I think this test will still pass if we remove the BinaryVersionOverride

@celiala celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 8d1bdb4 to 2521803 Compare August 11, 2022 22:21
Copy link
Collaborator Author

@celiala celiala left a comment

Choose a reason for hiding this comment

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

TFTRs!

Only thing from me is I'd like to keep the cleanup on fail backup test.

Ack:

  • left tests in restore_old_versions_test.go
  • updated comments

I'll go ahead and merge once gets pass.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @postamar and @RichardJCai)


pkg/ccl/backupccl/restore_old_versions_test.go line 1104 at r1 (raw file):
Ack.

if we remove the BinaryVersionOverride

I wasn't clear what to do for this?

But for now I've replaced PublicSchemasWithDescriptors with TODOPreV22_1 so that we can keep the test while still removing the gate.

Code quote:

sqlDB.CheckQueryResults(t, `SELECT x FROM test.public.t`, [][]string{{"3"}

pkg/sql/rename_table.go line 174 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

This comment needs to be rewritten also.

Done.


pkg/sql/importer/import_job.go line 131 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

This comment could be rewritten as "The public schema is expected to always be present in the database".

Done.

@celiala celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 2521803 to 5e26a84 Compare August 11, 2022 23:04
@celiala
Copy link
Collaborator Author

celiala commented Aug 15, 2022

These tests are failing - @postamar @RichardJCai - could you weigh-in on whether these are flakes or legit? TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes seems to require a test adjustment, but I'm unsure of how? (also, feel free to take-over/add to this PR as needed, if easier :) )

Bazel Essential CI:

  • pkg/ccl/serverccl/serverccl_test_/serverccl_test
    • exit code 142

GitHub CI:

Bazel Extended CI:

  • No pass/skip/fail event found for test
    • TestMetadataSST
------- Stdout: -------
=== RUN   TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
    ttljob_test.go:546: test case: ttljob_test.testCase{desc:"one column pk multiple nodes", createTable:"CREATE TABLE tbl (\n\tid UUID PRIMARY KEY DEFAULT gen_random_uuid(),\n\ttext TEXT\n) WITH (ttl_expire_after = '30 days')", preSetup:[]string(nil), postSetup:[]string(nil), numExpiredRows:1001, numNonExpiredRows:5, numSplits:10, forceNonMultiTenant:false, numNodes:5, expirationExpression:"", addRow:(func(*ttljob_test.rowLevelTTLTestJobTestHelper, *tree.CreateTable, time.Time))(nil)}
*
* INFO: Running test with the default test tenant. If you are only seeing a test case failure when this message appears, there may be a problem with your test case running within tenants.
*
    ttljob_test.go:679:
          Error Trace:  ttljob_test.go:679
          Error:        Not equal:
                        expected: 1001
                        actual  : 1002
          Test:         TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
    --- FAIL: TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes (13.87s)

@celiala
Copy link
Collaborator Author

celiala commented Aug 17, 2022

Closing in favor of #86225

@celiala celiala closed this Aug 18, 2022
@celiala celiala reopened this Aug 22, 2022
@celiala celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 5e26a84 to 0fe61f4 Compare August 22, 2022 21:35
Release note: None
Release justification: remove old version gates
@celiala celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 0fe61f4 to 4d6e876 Compare August 23, 2022 01:46
@celiala
Copy link
Collaborator Author

celiala commented Aug 23, 2022

TFTRs!

Incorporated additions from #86225 to get PR to pass - thank you! 🙏🏼

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2022

Build succeeded:

@craig craig bot merged commit 281e642 into cockroachdb:master Aug 23, 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.

4 participants