feat(xds): add public APIs for xds-client #2464
Conversation
|
@YutaoMa CI is failing because we have a job that checks that we don't expose certain APIs publically so this should be addressed https://github.com/hyperium/tonic/actions/runs/20112284748/job/57712601774?pr=2464#step:7:69 ideally, we should not expose things publically that we don't own to avoid potential breaking changes in the future. |
|
@LucioFranco as suggested I've removed the dependency on the pre-1.0 |
gu0keno0
left a comment
There was a problem hiding this comment.
Some general questions, LGTM overall
LucioFranco
left a comment
There was a problem hiding this comment.
LGTM! Feel free to address things in follow ups, nice to see the start here!
xds-client/src/client/watch.rs
Outdated
| /// } | ||
| /// } | ||
| /// ``` | ||
| pub fn next(&mut self) -> impl Future<Output = Option<ResourceEvent<T>>> + '_ { |
There was a problem hiding this comment.
take a look at some of the tokio::sync apis for what this signature should look like in regards to async fn/return types etc in general tokio is a great reference point
| /// A bidirectional stream for xDS ADS communication. | ||
| pub trait TransportStream: Send + 'static { | ||
| /// Send a discovery request to the server. | ||
| fn send(&mut self, request: DiscoveryRequest) -> impl Future<Output = Result<()>> + Send; |
There was a problem hiding this comment.
there might be a crate that provides macro sugar for the sendable async trait fn
There was a problem hiding this comment.
dfawley
left a comment
There was a problem hiding this comment.
Sorry for the late review. In general this looks great, thanks! I have a few changes to suggest, and feel free to punt on some of them until later.
| /// A discovery request to send to the xDS server. | ||
| #[derive(Debug, Clone)] | ||
| pub struct DiscoveryRequest { | ||
| // TODO: Add fields as needed | ||
| } | ||
|
|
||
| /// A discovery response from the xDS server. | ||
| #[derive(Debug, Clone)] | ||
| pub struct DiscoveryResponse { | ||
| // TODO: Add fields as needed | ||
| } |
There was a problem hiding this comment.
Is it possible to leave the transport as a byte transport, or does that not work with tonic so well?
The main design idea is that it's able to send generic messages (but not have to say "these are 'proto messages'" since "what a proto message is" will likely change from prost to google-protobuf). So we serialize inside the xdsclient and send the serialized bytes to the transport. It's still transmitted as proto, it just doesn't need to do the serialization in tonic.
| /// Creates a new bidirectional ADS stream to the xDS server. | ||
| /// | ||
| /// This may be called multiple times for reconnection. | ||
| fn connect(&self) -> impl Future<Output = Result<Self::Stream>> + Send; |
There was a problem hiding this comment.
Could you please rename this? Maybe to new_stream or create_stream? "Connect" implies there's a new TCP connection being made, but this just creates a stream.
| /// } | ||
| /// } | ||
| /// ``` | ||
| pub async fn next(&mut self) -> Option<ResourceEvent<T>> { |
There was a problem hiding this comment.
The other gRPC versions of this have a callback that is used to indicate the returned result has "finished being processed". Generally, what that means is: it's added a watch for all the related resources it think it will need. IIRC this was useful to batch these subscription requests as we block all ADS stream operations while calling the watchers. In Go we struggled for awhile with whether we should just say "the function blocks until processing is done" or "it's useful to pass this closure elsewhere to allow all watchers to be doing some async processing at the same time" and we eventually decided on the latter. In Rust this could be a oneshot channel or some equivalent that is just a signal that something is finished.
| pub fn watch<T: Resource>(&self, _name: impl Into<String>) -> ResourceWatcher<T> { | ||
| todo!() | ||
| } | ||
| } |
There was a problem hiding this comment.
We will also need the dump_resources method eventually in order to feed CSDS for debugging (https://pkg.go.dev/google.golang.org/[email protected]/internal/xds/clients/xdsclient#XDSClient.DumpResources).
Motivation
As explained previously #2459 , we are upstreaming an implementation of xDS client in Rust, in order to support xDS based routing and load balancing in tonic / gRPC Rust eventually. This PR is a continuation of the effort, by exposing all the expected public APIs (traits, types, function signatures) first to lay out the design.
Solution
Here is an overview of all the public APIs we are adding in this PR:
error.rsError,Resultresource.rsResourcetraittransport.rsTransport,TransportStream,DiscoveryRequest,DiscoveryResponseruntime.rsRuntimetraitconfig.rsClientConfigwatcher.rsResourceWatcher<T>,ResourceEvent<T>client.rsXdsClient,XdsClientBuilderIn case the reviewer is curious if the provided APIs could support the expected functionality of an xDS client, I have a working proof of concept that implements these APIs in my fork: https://github.com/YutaoMa/tonic/tree/yutaoma/xds-client-poc, it showcases the APIs working together with an implementation based on the
tonicstack (tonic,tokio,prost).