testserver: remove direct casts to *server.TestServer
#107866
Conversation
|
This change is |
knz
force-pushed
the
20230729-avoid-ts-cast
branch
from
8517275 to
31f98a0
Compare
knz
requested review from
nkodali,
adityamaru,
samiskin,
herkolategan and
smg260
and removed request for
a team
There was a problem hiding this comment.
Last two commits
Reviewed 10 of 10 files at r4, 19 of 99 files at r5, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @knz, and @stevendanna)
-- commits line 44 at r4:
Is it because GossipI is part of the storage layer interface?
pkg/cli/testutils.go line 52 at r5 (raw file):
// TestCLI wraps a test server and is used by tests to make assertions about the output of CLI commands. type TestCLI struct { // a copy of the insecure mode parameter.
nit: should start with the capital letter.
knz
force-pushed
the
20230729-avoid-ts-cast
branch
from
0414edb to
50e86f3
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Is it because
GossipIis part of the storage layer interface?
It's more because GossipI is an interface method, and .Gossip is not (because of package dependency cycle).
Same about .Stores vs .GetStores which the next commit also simplifies.
pkg/cli/testutils.go line 52 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: should start with the capital letter.
Done.
knz
force-pushed
the
20230729-avoid-ts-cast
branch
from
50e86f3 to
6f29b21
Compare
107868: rpc,*: sanitize the configuration of rpc.Context r=yuzefovich,stevendanna a=knz The main goal of this change is to offer a `.RPCClientConn()` method on test servers. To achieve this, it was necessary to lift the code previously in `cli.getClientGRPCConn` into a more reusable version of it, now hosted in `rpc.NewClientContext()`. I also took the opportunity to remove the dependency of `rpc.Context` on `base.Config`, by spelling out precisely which fields are necessary to RPC connections. Numerous tests could be simplified as a result. Needed for #107866. Epic: CRDB-18499 Co-authored-by: Raphael 'kena' Poss <[email protected]>
Tests should use `.GossipI()` Release note: None
We want to keep tests using go interfaces, to ensure the test server APIs can be mocked if needed. Incidentally, this patch improves the APIs as follows: - the health probe for secondary tenant servers now properly includes the draining state of the RPC interface (previously, only the SQL draining state was included). - `ApplicationLayerInterface` now includes `Readiness()` and `DefaultZoneConfig()`. - `StorageLayerInterface` now includes `ScratchRangeWithExpirationLease()`, `GetRangeLease()`, `TsDB()`, `Locality()` and `DefaultSystemZoneConfig()`. Release note: None
Release note: None
Release note: None
knz
force-pushed
the
20230729-avoid-ts-cast
branch
from
6f29b21 to
eaf0f22
Compare
|
bors r=yuzefovich,stevendanna |
|
Canceled. |
|
bors r=yuzefovich,stevendanna |
knz
force-pushed
the
20230729-avoid-ts-cast
branch
from
2b29313 to
eaf0f22
Compare
|
Canceled. |
|
bors r=yuzefovich,stevendanna |
|
Build succeeded: |
First 3 commits from #107868.
In a later stage we would like the result of
StartServerto return different concrete types depending on parameters. Before we can do that however, we need to remove direct casts of that return value to*server.TestServer.Incidentally, this patch improves the APIs as follows:
the health probe for secondary tenant servers now properly
includes the draining state of the RPC interface (previously,
only the SQL draining state was included).
ApplicationLayerInterfacenow includesReadiness()andDefaultZoneConfig().StorageLayerInterfacenow includesScratchRangeWithExpirationLease(),GetRangeLease(),TsDB(),Locality()andDefaultSystemZoneConfig().Epic: CRDB-18499