Skip to content

sql: use collection in zone config hydration#92035

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
chengxiong-ruan:use-collection-in-zone-config-hydration
Dec 5, 2022
Merged

sql: use collection in zone config hydration#92035
craig[bot] merged 1 commit intocockroachdb:masterfrom
chengxiong-ruan:use-collection-in-zone-config-hydration

Conversation

@chengxiong-ruan
Copy link
Copy Markdown
Contributor

@chengxiong-ruan chengxiong-ruan commented Nov 17, 2022 •
edited
Loading

part of #88571

Previously, we use a getKey function (which is basically a
kv lookup) to fetch zone configs and table descriptors required
for zone config hydration. It has redundant work of deserializing
protobufs done by descriptor collection. This commit change it
to use a helper interface to lookup zoneconfig and descriptor
directly. This also make sure that we ustilize in cache zone configs
when possible.

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Nov 17, 2022

This change is Reviewable

@chengxiong-ruan chengxiong-ruan force-pushed the use-collection-in-zone-config-hydration branch 11 times, most recently from 9bae231 to 586676c Compare November 18, 2022 17:12
@chengxiong-ruan chengxiong-ruan force-pushed the use-collection-in-zone-config-hydration branch 4 times, most recently from 87135e3 to d0dc347 Compare December 1, 2022 20:37
@chengxiong-ruan chengxiong-ruan marked this pull request as ready for review December 1, 2022 20:40
@chengxiong-ruan chengxiong-ruan requested a review from a team as a code owner December 1, 2022 20:40
@chengxiong-ruan chengxiong-ruan requested a review from a team December 1, 2022 20:40
@chengxiong-ruan chengxiong-ruan requested review from a team as code owners December 1, 2022 20:40
@chengxiong-ruan chengxiong-ruan requested review from a team and benbardin and removed request for a team and benbardin December 1, 2022 20:40
postamar
postamar suggested changes Dec 2, 2022
View reviewed changes
Copy link
Copy Markdown

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

This should probably be reviewed by @ajwerner who understands zone configs better than I do. I noticed a few things.

One thing I'm curious about is having the gossiped system config provide zone configs: is this necessary? I thought I heard at some point that we didn't gossip the system config any more.

@chengxiong-ruan
Copy link
Copy Markdown
Contributor Author

chengxiong-ruan commented Dec 2, 2022

One thing I'm curious about is having the gossiped system config provide zone configs: is this necessary? I thought I heard at some point that we didn't gossip the system config any more.

@postamar sorry not familiar with this part, I'm curious that which change caused your curiosity on this?

@postamar
Copy link
Copy Markdown

postamar commented Dec 2, 2022

I'm curious to the possibility that the code relying on the systemZoneConfigHelper might already be dead code, that's all.

@chengxiong-ruan
Copy link
Copy Markdown
Contributor Author

chengxiong-ruan commented Dec 2, 2022

I'm curious to the possibility that the code relying on the systemZoneConfigHelper might already be dead code, that's all.

Oh, I just looked at this a bit and some of the kv code to get span configs can still trace down to this.

@chengxiong-ruan chengxiong-ruan force-pushed the use-collection-in-zone-config-hydration branch from d0dc347 to e579eaa Compare December 2, 2022 17:30
@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Dec 2, 2022

I'm curious to the possibility that the code relying on the systemZoneConfigHelper might already be dead code, that's all.

Oh, I just looked at this a bit and some of the kv code to get span configs can still trace down to this.

Can you spell the path out for me?

@chengxiong-ruan
Copy link
Copy Markdown
Contributor Author

chengxiong-ruan commented Dec 2, 2022

Can you spell the path out for me?

@ajwerner here is an example path:

cockroach/pkg/sql/zone_config.go

Line 189 in 9fc656e

func zoneConfigHook(

cockroach/pkg/sql/zone_config.go

Line 41 in 9fc656e

config.ZoneConfigHook = zoneConfigHook

cockroach/pkg/config/system.go

Line 434 in 9fc656e

hook := ZoneConfigHook

cockroach/pkg/config/system.go

Line 308 in 9fc656e

entry, err := s.getZoneEntry(codec, id)

cockroach/pkg/config/system.go

Line 339 in 9fc656e

id, zone, err := s.getZoneConfigForKey(keys.SystemSQLCodec, key)

cockroach/pkg/kv/kvserver/replica_command.go

Line 3260 in 9fc656e

conf, err := confReader.GetSpanConfigForKey(ctx, startKey)

@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Dec 2, 2022

Alrighty, I think I have a gift for you. We can remove a bunch of code.

@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Dec 2, 2022

#92941 I'm certain there's some tests to spruce up, but I think it's legit. It'll wrap up a whole lot of work. h/t to @arulajmani and @irfansharif!

@chengxiong-ruan chengxiong-ruan force-pushed the use-collection-in-zone-config-hydration branch from e579eaa to fd65b44 Compare December 2, 2022 18:45
@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Dec 5, 2022

Let's merge this and then later delete the code. There's a lot of tests to fix up.

ajwerner
ajwerner approved these changes Dec 5, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Reviewed 13 of 27 files at r1, 4 of 7 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @postamar)

pkg/sql/catalog/descs/descriptor.go line 86 at r3 (raw file):

	ctx context.Context, txn *kv.Txn, id descpb.ID,
) (catalog.TableDescriptor, error) {
	// Ignore ids with not descriptor.

nit: without a descriptor.

@chengxiong-ruan
sql: use collection in zone config hydration
0c08f56 
part of cockroachdb#88571

Previously, we use a `getKey` function (which is basically a
kv lookup) to fetch zone configs and table descriptors required
for zone config hydration. It has redundant work of deserializing
protobufs done by descriptor collection. This commit change it
to use a helper interface to lookup zoneconfig and descriptor
directly. This also make sure that we ustilize in cache zone configs
when possible.

Release note: None
@chengxiong-ruan chengxiong-ruan force-pushed the use-collection-in-zone-config-hydration branch from fd65b44 to 0c08f56 Compare December 5, 2022 15:12
@chengxiong-ruan
Copy link
Copy Markdown
Contributor Author

chengxiong-ruan commented Dec 5, 2022

TFTR!
bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Dec 5, 2022

👎 Rejected by code reviews

@chengxiong-ruan
Copy link
Copy Markdown
Contributor Author

chengxiong-ruan commented Dec 5, 2022

@postamar still need you stamp because there was a change requested. Thanks.

@chengxiong-ruan
Copy link
Copy Markdown
Contributor Author

chengxiong-ruan commented Dec 5, 2022

TFTR!
bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Dec 5, 2022

Build succeeded:

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

Reviewers

2 more reviewers

@ajwerner ajwerner ajwerner approved these changes

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

4 participants

@chengxiong-ruan @cockroach-teamcity @postamar @ajwerner