spanconfigstore: introduce a system span config store#76871
spanconfigstore: introduce a system span config store#76871craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
adityamaru
left a comment
There was a problem hiding this comment.
I still have to look at test files, will do that in between meetings. No blocking comments yet. I really like how "simple" this change is, I was expecting something much scarier but I guess most of that was frontloaded.
| case update.Target.IsSystemTarget(): | ||
| systemSpanConfigStoreUpdates = append(systemSpanConfigStoreUpdates, update) | ||
| default: | ||
| panic("unknown target type") |
There was a problem hiding this comment.
worth returning an error instead, considering the signature already supports it?
|
|
||
| deletedSystemTargets, addedSystemSpanConfigRecords, err := s.mu.systemSpanConfigStore.apply( | ||
| systemSpanConfigStoreUpdates..., | ||
| ) |
pkg/spanconfig/systemtarget.go
Outdated
| switch st.systemTargetType { | ||
| case SystemTargetTypeAllTenantKeyspaceTargetsSet: | ||
| if st.targetTenantID != nil { | ||
| if !st.targetTenantID.Equal(roachpb.TenantID{}) { |
There was a problem hiding this comment.
Can we use the IsSet method in tenant.go here and everywhere else we perform these comparisons with roachpb.TenantID{}?
| } | ||
|
|
||
| // combine takes a key and an associated span configuration and combines it with | ||
| // all system span configs that apply to the given key. |
| return roachpb.SpanConfig{}, err | ||
| } | ||
| // Construct a list of system targets that apply to the key given its tenant | ||
| // prefix. |
There was a problem hiding this comment.
nit: Maybe explicity write out the three targets we want to check against, as part of this comment? Might be helpful for future readers even though the constructor methods are quite intuitively named now.
adityamaru
left a comment
There was a problem hiding this comment.
Tests also LGTM to me!
| // Ten 10 -> Ten 10: 200 | ||
| // Ten 20 -> Ten 20: 300 | ||
| // | ||
| // Span config: 1 |
There was a problem hiding this comment.
am I just missing it or is this SpanConfig entry missing?
There was a problem hiding this comment.
Oh or is this the SpanConfig we combine with below?
This patch introduces an in-memory datastructure to keep track of system span configurations. This thing is consulted every time we fetch the span configuration for a key to hydrate PTS information. In particular, when fetching span configurations, we combine PTS information stored on any system span configurations that may apply to the given key with the PTS information stored on the key's associated "regular" span config. Release note: None
0aa60fe to
d11b185
Compare
arulajmani
left a comment
There was a problem hiding this comment.
TFTR! Will bors on green
Dismissed @adityamaru from 3 discussions.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @irfansharif)
pkg/spanconfig/systemtarget.go, line 192 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
Can we use the IsSet method in tenant.go here and everywhere else we perform these comparisons with roachpb.TenantID{}?
Good call; done.
pkg/spanconfig/spanconfigstore/store.go, line 138 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
worth returning an error instead, considering the signature already supports it?
Done.
pkg/spanconfig/spanconfigstore/store.go, line 159 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
missing err check
Done.
pkg/spanconfig/spanconfigstore/systemspanconfigstore.go, line 93 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
nit: Maybe explicity write out the three targets we want to check against, as part of this comment? Might be helpful for future readers even though the constructor methods are quite intuitively named now.
Done
pkg/spanconfig/spanconfigstore/systemspanconfigstore_test.go, line 56 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
Oh or is this the SpanConfig we
combinewith below?
Yup, exactly, it's the SpanConfig I'm combining with below.
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
This patch introduces an in-memory datastructure to keep track of
system span configurations. This thing is consulted every time we fetch
the span configuration for a key to hydrate PTS information. In
particular, when fetching span configurations, we combine PTS
information stored on any system span configurations that may apply to
the given key with the PTS information stored on the key's associated
"regular" span config.
Release note: None