sql: add TenantStatusServer.NodeLocality to support crdb_internal.ranges{_no_leases} for secondary tenants #92293
Conversation
|
This change is |
There was a problem hiding this comment.
LGTM with nits.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ecwall)
-- commits line 13 at r1:
nit: you do not need to include the full URL here, just Epic: CRDB-14522 is sufficient.
pkg/server/status.go line 1384 at r1 (raw file):
} // NodeLocality implements the serverpb.Status interface
nit: period at end of comment.
pkg/server/serverpb/status.go line 84 at r1 (raw file):
// the SQL system to query for the node locality of a given node ID. // It is available for tenants. type NodeLocalityServer interface {
Could you discuss with @dhartunian whether this interface can be merged with those above (in fact, also whether the others could be merged together). At this point I do not understand why we have so many interfaces and why they need to be separate.
There was a problem hiding this comment.
Reviewable status:
pkg/server/serverpb/status.go line 84 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Could you discuss with @dhartunian whether this interface can be merged with those above (in fact, also whether the others could be merged together). At this point I do not understand why we have so many interfaces and why they need to be separate.
@ecwall I agree in this case that we should avoid introducing a new interface. It looks like RegionsServer above and TenantStatusServer are used in the same contexts so I'd suggest either:
- merging all 3 into
TenantStatusServerto unify all the stuff tenants need from aConnector
or - add your
NodeLocalitymethod toTenantStatusServerand put a TODO onRegionsServerto merge later
I don't see anything in particular gained from a separate interface here. It does add confusion when the list of config params continues to grow, especially when multiple args are just the same struct fulfilling various single-method interfaces.
There was a problem hiding this comment.
Reviewable status:
Previously, knz (Raphael 'kena' Poss) wrote…
nit: you do not need to include the full URL here, just
Epic: CRDB-14522is sufficient.
Done.
pkg/server/status.go line 1384 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit: period at end of comment.
Done.
pkg/server/serverpb/status.go line 84 at r1 (raw file):
Previously, dhartunian (David Hartunian) wrote…
@ecwall I agree in this case that we should avoid introducing a new interface. It looks like
RegionsServerabove andTenantStatusServerare used in the same contexts so I'd suggest either:
- merging all 3 into
TenantStatusServerto unify all the stuff tenants need from aConnector
or- add your
NodeLocalitymethod toTenantStatusServerand put a TODO onRegionsServerto merge laterI don't see anything in particular gained from a separate interface here. It does add confusion when the list of config params continues to grow, especially when multiple args are just the same struct fulfilling various single-method interfaces.
Created a PR to merge the preexisting TenantStatusServer and RegionsServer here #92599. I'll rebase this PR on top of that after it merges.
ecwall
changed the title
There was a problem hiding this comment.
Reviewed 14 of 14 files at r3, 14 of 14 files at r4, all commit messages.
Reviewable status:
pkg/ccl/kvccl/kvtenantccl/connector.go line 421 at r4 (raw file):
resp, err = c.Regions(ctx, req) return err }); err != nil {
nit:
err := c.withClient(...)
return resp, errpkg/ccl/kvccl/kvtenantccl/connector.go line 436 at r4 (raw file):
resp, err = c.NodeLocality(ctx, req) return err }); err != nil {
ditto
…ges{_no_leases} for secondary tenants
This PR adds a new node locality lookup KV admin function which is
used by crdb_internal.ranges{_no_leases} and SHOW RANGES to
work for secondary tenants.
Release note: None
Epic: CRDB-14522
|
bors r=knz |
|
Build succeeded: |
This PR adds a new node locality lookup KV admin function which is
used by crdb_internal.ranges{_no_leases} and SHOW RANGES to
work for secondary tenants.
Release note: None
Epic: CRDB-14522