feat: add an in-process worker transport (no gRPC)#548
Conversation
The prost message types carry no tonic dependency, so a transport that is not gRPC can speak the same wire shape without pulling in the gRPC stack. Only the tonic client and server stay gated; the generator emits those gates so a regeneration cannot drop them. tonic-prost feeds only the generated client and server, so it moves behind the feature too. Co-authored-by: Stu Hood <stuhood@gmail.com>
The benchmarks crate's dev-dependency on the lib re-unified grpc into every test build, so a genuine no-gRPC test run was impossible; the dataset suites move into the benchmarks crate and the gRPC-coupled test utilities gate behind grpc. A unit-test-no-grpc job then runs the whole lib suite with the feature off. Co-authored-by: Stu Hood <stuhood@gmail.com>
InProcessChannelResolver routes the three protocol methods straight to a co-located Worker, with no gRPC, no IPC, and no serialization round-trip: the reference implementation of the protocol for a co-located worker, and the first transport that exercises the abstraction with grpc off. Its end-to-end test (a distributed GROUP BY across tasks) runs under the no-gRPC CI job. Co-authored-by: Stu Hood <stuhood@gmail.com>
be44b9c to
0dfe291
Compare
|
Thanks @stuhood. Pulled |
|
@gabotechs Can you please take a look? |
gabotechs
left a comment
There was a problem hiding this comment.
Before reviewing, I see this PR contains a lot of changes that are unnecessary, for example:
- Tweaks to the CI. I'd leave the CI as it is for now, that can be evaluated separately
- Changes to the public API, like exposes the
metrics_storemethod in DistributedExec - Move some integration tests to the benchmarks
Before reviewing, I'd recommend to really scope the PR just to have an in-memory implementation of the WorkerChannel.
Also, as I don't think there's any external use-case, it might be better to delay adding more WorkerChannel implementations to when they are needed. The WorkerChannel abstraction is in early stages, and it will likely suffer several breaking changes in the short term as we try to accommodate more features. Having more implementations that this project is not going to use will mean paying the price of maintaining those implementations with no benefit, so I'd recommend to defer this work a bit until there's a valid use case.
For example, when we have AQE re-optimizing queries on the fly, there'll be a use case why people would want to run this project in a fully in-memory fashion, once we get that it might be a good time to start thinking about officially supporting more implementations in this project.
I agree: I'll attempt to scope this down to only adding the new
I think that having two implementations of |
0dfe291 to
36af89b
Compare
What
This PR makes the
WorkerChannelabstraction usable by a transport that is not gRPC, and adds the in-process reference transport that proves it.Why
#512 abstracted the worker protocol, but gRPC stayed the only implementation and the proto sat behind the
grpcfeature, so nothing built or ran without it.How
One commit per concern, each with its own test:
grpc. Only the tonic client and server stay gated, and the generator emits those gates.benchmarksdev-dependency (it re-unifiedgrpcinto every test build) and added aunit-test-no-grpcCI job that runs the whole lib suite with the feature off.InProcessChannelResolver: routes the three protocol methods to a co-locatedWorker. No gRPC, no IPC, no serialization round trip.metrics_store()with a publicinsert, and a no-gRPCdecode_task_metrics: a driver whose transport returns metrics out-of-band files the decoded frames into the store the EXPLAIN rewrite reads. Happy to break this one out into its own PR too if preferred.Tests
The in-process transport runs a distributed
GROUP BYend to end withgrpcoff in CI, and a unit test pins the metrics frame-to-store path.