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: make descs.Collection usage more consistent #64054

Merged

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Apr 22, 2021

This commit removes some undesirable uses of catalogkv in schema change logic.

Relates to #64134.

Release note: None

@ajwerner ajwerner requested a review from a team April 22, 2021 05:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/descs-collection-schema-changer branch from d4867a4 to 1a0c8de Compare April 22, 2021 06:05
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, this is nice.

Can we have an issue for this please? I did a regex search for catalogkv.(Must)?Get\w+Desc\w+\( on master and got 68 hits, the vast majority in the sql package. It looks like a good chunk of these are easily replaceable with Collection methods.

pkg/sql/schema_changer_test.go Outdated Show resolved Hide resolved
This commit removes undesirable uses of catalogkv.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/descs-collection-schema-changer branch from 1a0c8de to db38bdd Compare April 23, 2021 16:53
@ajwerner
Copy link
Contributor Author

Can we have an issue for this please? I did a regex search for catalogkv.(Must)?Get\w+Desc\w+( on master and got 68 hits, the vast majority in the sql package. It looks like a good chunk of these are easily replaceable with Collection methods.

#64134.

@ajwerner
Copy link
Contributor Author

TFTR

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 23, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Apr 23, 2021

Build succeeded:

@craig craig bot merged commit dd8fa28 into cockroachdb:master Apr 23, 2021
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