spanconfigstore: introduce a system span config store #76871
Conversation
|
This change is |
| 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
| @@ -189,7 +189,7 @@ func (st SystemTarget) encode() roachpb.Span { | |||
| func (st SystemTarget) validate() error { | |||
| 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.
| // 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
arulajmani
force-pushed
the
protectedts-system-spanconfig-store
branch
from
0aa60fe
to
d11b185
Compare
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