serverutils: clean up some interfaces#107612
Conversation
|
This change is |
stevendanna
left a comment
There was a problem hiding this comment.
I like the rename of TenantOrServer for sure and I also like the addition of SystemLayer().
I could see an argument for keeping an embedded ApplicationLayerInterface if we find transitioning tests to this API too difficult, but I'd prefer that we get to this more explicit access if we can.
c4299eb to
1836bf2
Compare
c229b4e to
519f9f9
Compare
018d40c to
f90d53e
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @herkolategan, @shermanCRL, @smg260, and @yuzefovich)
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: probably worth calling out the change in
ServingSQLAddr(which was the only place that had different logic when default test tenant was started), removal ofHostSQLAddrmethod, and inclusion ofQueryTableIDand friends methods.
Done.
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: did you mean "important interfaces"?
Indeed. Thanks
pkg/server/testserver.go line 462 at r4 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: XXes on SQLInstanceID and StatusServer in the first commit.
Fixed.
pkg/testutils/serverutils/api.go line 466 at r7 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: should
TenantCapabilitiesReaderbe inTenantControlInterface?
I don't feel it should - it's not a control interface.
pkg/testutils/serverutils/test_server_shim.go line 161 at r4 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: perhaps for the follow-up, but should we rename
base.TestTenantArgsand alike too?
Let's do it in a different PR.
f90d53e to
5f6f03c
Compare
5f6f03c to
3c71b07
Compare
3c71b07 to
77fefc4
Compare
herkolategan
left a comment
There was a problem hiding this comment.
Lovely separation of concerns, and a welcome improvement. I think this will help overall readability in the tests as well. Didn't see anything I don't agree with. Thanks for doing this!
Reviewed 70 of 102 files at r3, 3 of 20 files at r4, 3 of 117 files at r8, 115 of 115 files at r12, 1 of 1 files at r13, 112 of 112 files at r14, 2 of 2 files at r15, 4 of 4 files at r16, all commit messages.
Reviewable status:
yuzefovich
left a comment
There was a problem hiding this comment.
Nice work!
Reviewed 1 of 117 files at r8, 7 of 115 files at r12.
Reviewable status:
In this commit:
- `TestTenantInterface` renamed to `ApplicationLayerInterface`.
- `TestServerInterface` split into:
- `StorageLayerInterface` containing most of the methods previously
in `TestServerInterface`.
- `TenantControlInterface` with just the methods relating to
starting the service for secondary tenants.
- Relevant methods moved from `TestServerInterface` to
`ApplicationLayerInterface`:
- `ServingSQLAddr()` - renamed to `AdvSQLAddr()`
- `RPCAddr()`
- `LeaseManager()`
- `DistSenderI()`
- `MigrationServer()`
- `SetDistSQLSpanResolver()`
- `CollectionFactory()`
- `SystemTableIDResolver()`
- Removed `HostSQLAddr()` - use `.SystemLayer().AdvSQLAddr()` instead.
- `AdvSQLAddr()` (previously `ServingSQLAddr()`) was
simplified to always return the advertised SQL address for the
current layer.
- Updated comments/docstrings for the interfaces.
Release note: None
Prior to this commit, we had methods on TestServer called "ServingXXXAddr" but under the hood were referring to the advertised addresses in the config struct. This was confusing. This patch updates the name of the methods to use "Adv" to be coherent with the field they are exposing. Release note: None
Release note: None
Release note: None
77fefc4 to
b2992d7
Compare
|
Let's try it! bors r=yuzefovich,stevendanna single on |
|
Looks like this PR is still included in a batch with others. Perhaps "bors r=..." and "single on" don't work together ref. bors r+ single on |
|
Already running a review |
|
bors r- |
|
Canceled. |
|
bors r+ single on |
|
Timed out. |
|
bors r+ p=99 single on |
|
Build succeeded: |
Epic: CRDB-18499
Part of solving #107058.
Informs #106772.
In this commit:
TestTenantInterfacerenamed toApplicationLayerInterface.TestServerInterfacesplit into:StorageLayerInterfacecontaining most of the methods previouslyin
TestServerInterface.TenantControlInterfacewith just the methods relating tostarting the service for secondary tenants.
Relevant methods moved from
TestServerInterfacetoApplicationLayerInterface:ServingSQLAddr()- renamed toAdvSQLAddr()RPCAddr()LeaseManager()DistSenderI()MigrationServer()SetDistSQLSpanResolver()CollectionFactory()SystemTableIDResolver()Removed
HostSQLAddr()- use.SystemLayer().AdvSQLAddr()instead.AdvSQLAddr()(previouslyServingSQLAddr()) wassimplified to always return the advertised SQL address for the
current layer.
Updated comments/docstrings for the interfaces.
Release note: None