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

sql/schemachanger: extend metadata updater to support for updating zone configs #83804

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Jul 5, 2022

This PR will extend the metadata updater to first support updating zone config, and next adopt
this infrastructure inside the existing zone config code.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@fqazi fqazi force-pushed the metaDataUpdateZC branch 3 times, most recently from 3b2f5b6 to 3dd75b3 Compare July 5, 2022 14:03
@fqazi fqazi marked this pull request as ready for review July 5, 2022 20:00
@fqazi fqazi requested a review from a team July 5, 2022 20:00
@fqazi fqazi requested a review from a team as a code owner July 5, 2022 20:00
@fqazi fqazi requested review from a team July 5, 2022 20:00
@fqazi fqazi requested a review from a team as a code owner July 5, 2022 20:00
@fqazi fqazi requested a review from dt July 5, 2022 20:00
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.

:lgtm: after you remove the unused codec

Reviewed 20 of 20 files at r1, 17 of 17 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt and @fqazi)


pkg/sql/descmetadata/metadata_updater.go line 46 at r1 (raw file):

	descriptors  *descs.Collection
	cacheEnabled bool
	codec        keys.SQLCodec

Is this used anymore?

Previously, there was no way for the declarative schema changer to set
zone configs without invoking into sql, which would lead to a circular
dependency. These changes add an interface for allow the declarative
schema changes to issue zone config updates via the MetadataUpdater.

Release note: None
@fqazi fqazi force-pushed the metaDataUpdateZC branch from 3dd75b3 to cb79e3a Compare July 7, 2022 00:25
Copy link
Collaborator Author

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

@ajwerner TFTR!

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


pkg/sql/descmetadata/metadata_updater.go line 46 at r1 (raw file):

Previously, ajwerner wrote…

Is this used anymore?

Done.

@fqazi fqazi force-pushed the metaDataUpdateZC branch from cb79e3a to 4cc738d Compare July 7, 2022 00:28
Previously, the code to update the zone configs was separate
between the declarative schema changer and the legacy schema
changer. This was inadequate because we have a opportunity
to share logic here. To address this the logic to update and
delete zone configurations is now shared.

Release note: None
@fqazi fqazi force-pushed the metaDataUpdateZC branch from 4cc738d to 3d90060 Compare July 7, 2022 04:11
@fqazi
Copy link
Collaborator Author

fqazi commented Jul 7, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 7, 2022

Build succeeded:

@craig craig bot merged commit f93ca3a into cockroachdb:master Jul 7, 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