rfc: Make DestinationServiceQuery generic #749

olix0r · 2018-04-12T17:36:12Z

The goals of this change are:

Reduce the size/complexity of control::discovery in order to ease code reviews.
Extract a reusable grpc streaming utility.

There are no intended functional changes.

control::discovery::DestinationServiceQuery is used to track the state of a request (and streaming response) to the destination service. Very little of this logic is specific to the destination service.

The DestinationServiceQuery and associated UpdateRx type have been moved to a new module, control::remote_stream, as Remote and Receiver, respectively. Both of these types are generic over the gRPC message type, so it will be possible to use this utility with additional API endpoints.

The Receiver::poll implementation has been simplified to be more idiomatic with the rest of our code (namely, using try_ready!).

Additionally, the enum states are now hidden behind an API. Remote now exposes constructors, a needs_reconnect() method that indicates whether the remote is disconnected, and a into_receiver_maybe() that consumes the remote, producing a receiver if one exists.

Note: It may help to review the individual commits in order.

Additionally, `UpdateRx` is now `Receiver`. `DestinationServiceQuery::connect_maybe` has been moved onto `DiscoveryWork`, where it is called from.

Make Remote and Receiver generic over the response future and body, instead of the http service.

To further simplify `control::discovery`, add a public API onto Remote, hiding the internal enum representation. Furthermore, Receiver now _only_ exposes a Stream and not its internal state.

briansmith · 2018-04-12T18:52:17Z

I understand that the goal here is to enable other implementations of service discovery with equivalent functionality to the Destination service.

I think a good way to validate this design would be to refactor the DNS-based service discovery to implement this interface. Or, if that's practical and we think DNS is special in some way, then we should document why we didn't redo DNS in this mold.

hawkw · 2018-04-12T22:43:25Z

@briansmith

I think a good way to validate this design would be to refactor the DNS-based service discovery to implement this interface.

I agree, but I think we ought to do this in a separate PR.

olix0r · 2018-04-12T22:48:41Z

I understand that the goal here is to enable other implementations of service discovery with equivalent functionality to the Destination service.

Not exactly. The goal is to enable interacting with arbitrary gRPC streaming endpoints. remote_stream models the state of such an endpoint (note that its M type must be prost::Message + Default and its F type is Future<Item = http::Response<Body>>, for example).

briansmith · 2018-04-19T18:40:09Z

proxy/src/control/remote_stream.rs

+pub struct Remote<M, F, B: Body = RecvBody>(Inner<M, F, B>);
+
+#[derive(Debug)]
+enum Inner<M, F, B: Body = RecvBody> {


Why is it necessary to have separate Remote and Inner types? Usually we would have an Inner type if if we have an Option<Inner> that we sometimes take(), but that's not the case here. If we can combine this into Remote, let's do so, to make the code easier to read. Otherwise, let's add a comment here about why it is split out.

briansmith · 2018-04-19T18:40:48Z

proxy/src/control/discovery.rs

+    /// Initiates a query `query` to the Destination service and returns it as
+    /// `Some(query)` if the given authority's host is of a form suitable for using to
+    /// query the Destination service. Otherwise, returns `None`.
+    pub fn connect_maybe(


This shouldn't be pub, AFAICT.

We should rename this function to connect_to_destination_service_maybe() since it is now in a more general context than before.

briansmith · 2018-04-19T18:45:27Z

proxy/src/control/discovery.rs

@@ -62,49 +63,11 @@ pub struct DiscoveryWork<T: HttpService<ResponseBody = RecvBody>> {

 struct DestinationSet<T: HttpService<ResponseBody = RecvBody>> {
    addrs: Exists<Cache<SocketAddr, ()>>,
-    query: Option<DestinationServiceQuery<T>>,
+    query: Option<Remote<PbUpdate, T::Future, T::ResponseBody>>,


I think we should have a type alias type DestinationServiceQuery<T> = Remote<PbUpdate, T::Future, T::ResponseBody> and use it in this module instead of Remote directly. Otherwise IMO the code would be harder to understand than it is now.

briansmith · 2018-04-19T18:53:01Z

proxy/src/control/discovery.rs

+                None => (None, Exists::Unknown),
+                Some(query) => {
+                    match query.into_receiver_maybe() {
+                        None => (Some(Remote::new()), Exists::Unknown),


It would be much clearer to just make the Remote enum public with public variants so that we can match on it directly. This code here only makes sense if Remote::new() is equivalent to what query was originally, which means we're not gaining anything by hiding the structure of Remote since, if there was a third variant of that enum, or if the enum had any different structure, this code would almost definitely be wrong.

briansmith · 2018-04-19T18:58:40Z

proxy/src/control/discovery.rs

Let's lose the extra whitespace changes.

briansmith · 2018-04-19T18:59:15Z

proxy/src/control/remote_stream.rs

@@ -0,0 +1,122 @@
+


No blank line up here.

briansmith · 2018-04-19T19:02:37Z

proxy/src/control/remote_stream.rs

+    M: Message + Default,
+    B: Body<Data = Data>,
+    F: Future<Item = http::Response<B>>,
+{


Again, here we're creating a big API with a bunch of boilerplate for, AFAICT, no reason, since there's no way to implement or use this API correctly unless Remote is an enum isomorphic to Some<Receiver<M, F, B>>. It would be better to lose all this boilerplate and make the code a lot easier to understand.

olix0r · 2018-05-01T21:36:36Z

@briansmith per your feedback, I've minimized the API change so that there's lesser impact to discovery.

The Remote type is now generic over the HttpService instead of its Future and body types -- this simplifies type signature complexity.

briansmith · 2018-05-03T18:20:11Z

proxy/src/control/discovery.rs

-                                    client,
-                                    vac.key(),
-                                    "connect");
+                            let query = Self::query_destination_service_if_valid_auth(


valid_auth seems like a poor name to me. Really the distinction isn't "valid" vs "invalid" but rather "may be an internal service" vs "is definitely not an internal service". I suggest query_destination_service_if_relevant or similar. In particular I would like to avoid implying anything about the validity of the authority in the name.

briansmith · 2018-05-03T18:23:56Z

proxy/src/control/discovery.rs

@@ -462,10 +427,10 @@ where
        for (auth, set) in &mut self.destinations {
            // Query the Destination service first.
            let (new_query, found_by_destination_service) = match set.query.take() {
-                Some(DestinationServiceQuery::ConnectedOrConnecting{ rx }) => {
+                Some(Remote::ConnectedOrConnecting{ rx }) => {


TIL we can't refer to this as DestinationServiceQuery because of rust-lang/rust#26264. (No change necessary.)

briansmith · 2018-05-03T18:28:51Z

proxy/src/control/discovery.rs

                },
                Ok(Async::NotReady) => {
-                    return (DestinationServiceQuery::ConnectedOrConnecting { rx }, exists);
+                    return (Remote::connected(rx), exists);


connected() is too misleading since the connection might not be connected yet. I suggest we just use ConnectedOrConnecting directly to make it clearer.

briansmith · 2018-05-03T18:34:56Z

proxy/src/control/remote_stream.rs

+        }
+    }
+
+    pub fn connected(rx: Receiver<M, S>) -> Self {


If you take my suggestion above then we should just remove connected().

briansmith · 2018-05-03T18:40:14Z

proxy/src/control/remote_stream.rs

+                Rx::Streaming(ref mut stream) => {
+                    return stream.poll().map_err(|e| grpc::Error::Grpc(match e {
+                        grpc::Error::Inner(()) => grpc::Status::UNKNOWN,
+                        grpc::Error::Grpc(status) => status,


I don't understand what the goal is with the remapping here and I found the structure confusing.

I suggest:

return stream.poll().map_err(|e| { match e { grpc::Error::Inner(()) => // <Document here why we are doing this.> grpc::Error::Grpc(grpc::Status::UNKNOWN), e => e, } });

The underlying issue here is the following:

grpc::Error<T> is generic over T: the underlying future's failure (for grpc::Error::Inner)

stream.poll can't actually fail with an Inner error, so it returns an grpc::Error<()>

here, we want to coerce the stream's error response to the general grpc error, so we need to drop the () type and convert to a T type.

This is why e => e doesn't work here -- the types are actually different.

i'll comment this a bit at least.

olix0r · 2018-05-08T21:46:39Z

@briansmith feedback addressed

The goals of this change are: 1. Reduce the size/complexity of `control::discovery` in order to ease code reviews. 2. Extract a reusable grpc streaming utility. There are no intended functional changes. `control::discovery::DestinationServiceQuery` is used to track the state of a request (and streaming response) to the destination service. Very little of this logic is specific to the destination service. The `DestinationServiceQuery` and associated `UpdateRx` type have been moved to a new module, `control::remote_stream`, as `Remote` and `Receiver`, respectively. Both of these types are generic over the gRPC message type, so it will be possible to use this utility with additional API endpoints. The `Receiver::poll` implementation has been simplified to be more idiomatic with the rest of our code (namely, using `try_ready!`).

olix0r added 3 commits April 12, 2018 16:52

Rename DestinationServiceQuery to Remote

558a7c1

Additionally, `UpdateRx` is now `Receiver`. `DestinationServiceQuery::connect_maybe` has been moved onto `DiscoveryWork`, where it is called from.

Make Remote and Receiver generic over a grpc message type

f200ff1

Move Remote and Receiver to a new remote_stream module

81e4df9

Make Remote and Receiver generic over the response future and body, instead of the http service.

olix0r added this to the 0.5.0 milestone Apr 12, 2018

olix0r requested a review from briansmith April 12, 2018 17:36

olix0r added the review label Apr 12, 2018

olix0r self-assigned this Apr 12, 2018

Add public APIs to Remote and Receiver

f586c09

To further simplify `control::discovery`, add a public API onto Remote, hiding the internal enum representation. Furthermore, Receiver now _only_ exposes a Stream and not its internal state.

olix0r force-pushed the ver/remote-stream branch from 54772e6 to f586c09 Compare April 12, 2018 17:45

briansmith reviewed Apr 19, 2018

View reviewed changes

briansmith suggested changes Apr 19, 2018

View reviewed changes

olix0r added 11 commits May 1, 2018 00:22

Merge branch 'master' into ver/remote-stream

ec4c8ee

Don't maintain a private Inner type, just use the public Remote type

ae459de

type alias DestinationQuery for clarity

86e5a05

comment

560ec22

use pattern-matching API and improve fn name

b2e3084

revert some api changes

ac1e4c0

Merge branch 'master' into ver/remote-stream

c6445d1

minimize API change in discovery

c8bf2d0

remove useless error type

a6e18c7

more uniform fn name

da44fed

whitespace

0b60800

briansmith reviewed May 3, 2018

View reviewed changes

olix0r added 3 commits May 8, 2018 21:06

Merge branch 'master' into ver/remote-stream

68d8f61

clarify fn name

ae86a7a

fixup and clarify grpc error conversion

c64041e

simpler receiver construction

2154020

briansmith approved these changes May 8, 2018

View reviewed changes

olix0r merged commit 8bedd9d into master May 8, 2018

olix0r deleted the ver/remote-stream branch May 8, 2018 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rfc: Make DestinationServiceQuery generic #749

rfc: Make DestinationServiceQuery generic #749

olix0r commented Apr 12, 2018

briansmith commented Apr 12, 2018

hawkw commented Apr 12, 2018 •

edited by olix0r

Loading

olix0r commented Apr 12, 2018

briansmith Apr 19, 2018

briansmith Apr 19, 2018

briansmith Apr 19, 2018

briansmith Apr 19, 2018

briansmith Apr 19, 2018

briansmith Apr 19, 2018

briansmith Apr 19, 2018

briansmith Apr 19, 2018

olix0r commented May 1, 2018

briansmith May 3, 2018

briansmith May 3, 2018

briansmith May 3, 2018

briansmith May 3, 2018

briansmith May 3, 2018

olix0r May 8, 2018

olix0r commented May 8, 2018

rfc: Make DestinationServiceQuery generic #749

rfc: Make DestinationServiceQuery generic #749

Conversation

olix0r commented Apr 12, 2018

briansmith commented Apr 12, 2018

hawkw commented Apr 12, 2018 • edited by olix0r Loading

olix0r commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r commented May 8, 2018

hawkw commented Apr 12, 2018 •

edited by olix0r

Loading