distsql: add plumbing for SQL pods to send RPCs to each other#76214
distsql: add plumbing for SQL pods to send RPCs to each other#76214craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
rharding6373
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @RaduBerinde)
pkg/sql/flowinfra/cluster_test.go, line 317 at r1 (raw file):
} func TestTenantClusterFlow(t *testing.T) {
This has a lot of code duplication with TestClusterFlow, but it turns out TestTenantInterface and TestServerInterface aren't compatible and this makes refactoring them a bit painful. I could spend more time refactoring this if there are strong feelings about it at this stage (and no one suggests to move this into the kvtennantccl package to eliminate the include).
RaduBerinde
left a comment
There was a problem hiding this comment.
CC @ajwerner
Nice work!!
Reviewable status:
pkg/ccl/kvccl/kvtenantccl/connector.go, line 603 at r1 (raw file):
} type PodConnector struct {
[nit] needs a comment
pkg/ccl/kvccl/kvtenantccl/connector.go, line 615 at r1 (raw file):
} func (p *PodConnector) GetNodeDescriptor(nodeID roachpb.NodeID) (*roachpb.NodeDescriptor, error) {
Instead of implementing this interface and passing it through kvtenant.AddressResolver, couldn't we directly implement a nodeDialer.AddressResolver? We wouldn't need to fake a NodeDescriptor.
I think nodeDialer.AddressResolver will eventually use a ServerID instead of NodeID (meaning that it can be either a SQLInstanceID or a NodeID depending on context).
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/kvccl/kvtenantccl/connector.go, line 615 at r1 (raw file):
Previously, RaduBerinde wrote…
Instead of implementing this interface and passing it through
kvtenant.AddressResolver, couldn't we directly implement anodeDialer.AddressResolver? We wouldn't need to fake aNodeDescriptor.I think
nodeDialer.AddressResolverwill eventually use aServerIDinstead ofNodeID(meaning that it can be either a SQLInstanceID or a NodeID depending on context).
➕
ad3624d to
6b17a36
Compare
rharding6373
left a comment
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/ccl/kvccl/kvtenantccl/connector.go, line 603 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] needs a comment
Removed.
pkg/ccl/kvccl/kvtenantccl/connector.go, line 615 at r1 (raw file):
Previously, ajwerner wrote…
➕
Done.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/flowinfra/cluster_test.go, line 317 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
This has a lot of code duplication with
TestClusterFlow, but it turns outTestTenantInterfaceandTestServerInterfacearen't compatible and this makes refactoring them a bit painful. I could spend more time refactoring this if there are strong feelings about it at this stage (and no one suggests to move this into thekvtennantcclpackage to eliminate the include).
I would leave it like this.
pkg/sql/flowinfra/cluster_test.go, line 15 at r2 (raw file):
import ( "context" dbsql "database/sql"
[nit] we usually use gosql as the alias for this
6b17a36 to
e42b6ac
Compare
rharding6373
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/flowinfra/cluster_test.go, line 15 at r2 (raw file):
Previously, RaduBerinde wrote…
[nit] we usually use
gosqlas the alias for this
Ah, I should have taken the linter seriously.
|
bors r+ |
This PR adds initial support for distributed flows in a multi-tenant cluster with multiple pods. It adds a test that assigns flows to multiple pods and executes them. In order to support communication between SQL pods in a multi-tenant environment, this PR adds a new kind of connector that can provide address resolution for SQL instances, and plumbs a node dialer that uses this new connector through distsql. If distsql is not in multi-tenant mode, the same node dialer is used whether SQL instances are communicating with other SQL instances or KV, as before this change. Release note: None
e42b6ac to
9f94ee3
Compare
|
Canceled. |
|
bors r- |
|
bors r+ |
|
Build succeeded: |
This PR adds initial support for distributed flows in a multi-tenant cluster
with multiple pods. It adds a test that assigns flows to multiple pods
and executes them. In order to support communication between SQL pods in
a multi-tenant environment, this PR adds a new kind of connector that
can provide address resolution for SQL instances, and plumbs a node
dialer that uses this new connector through distsql. If distsql is not
in multi-tenant mode, the same node dialer is used whether SQL instances
are communicating with other SQL instances or KV, as before this change.
Release note: None