feat(transport): Dynamic load balancing #341

dawid-nowak · 2020-05-05T14:43:55Z

Attempt to add a feature to Tonic so client side load balancing can be done over a
group of endpoints that change dynamically over time.

Motivation

An external service which is responsible for discovering new endpoints
and monitoring their health could make a decision to add/remove an
endpoint from the load balancing group in Tonic client. At the moment, the tonic only allows to load balance across endpoints from a list.

Solution

The solution is to expose a new method on Channel interface called balance_with_manager which will take a trait EventManager defined in Transport. EventManager will hold a lists of endpoint which need to be added/removed from balance service in tower.
The approach is very similar to existing Channel::balance_list.

##Comments
Comments on how to improve it are welcome :)

Attempt to add a feature to Tonic so client side load balancing can be done over a group of endpoints that change dynamically over time. An external service which is responsible for discovering new endpoints and monitoring their health could make a decision to add/remove an endpoint from the load balancing group in Tonic client. modified: examples/Cargo.toml new file: examples/src/load_balance_with_discovery/client.rs new file: examples/src/load_balance_with_discovery/server.rs modified: tonic/src/transport/channel/endpoint.rs modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/mod.rs modified: tonic/src/transport/service/discover.rs modified: tonic/src/transport/service/mod.rs

LucioFranco · 2020-05-07T19:13:10Z

examples/Cargo.toml

@@ -116,14 +124,14 @@ prost = "0.6"
 tokio = { version = "0.2", features = ["rt-threaded", "time", "stream", "fs", "macros", "uds"] }
 futures = { version = "0.3", default-features = false, features = ["alloc"] }
 async-stream = "0.2"
-tower = "0.3"
+tower = { version="0.3"}


Suggested change

tower = { version="0.3"}

tower = "0.3"

LucioFranco · 2020-05-07T19:17:50Z

This seems like an interesting start, I am a bit cautious about adding more complex types instead of just exposing the ability to pass in your own discover? I'd rather expose some sort of discover or channel hook than a endpoint manager. What do you think?

dawid-nowak · 2020-05-08T14:30:19Z

I agree that passing discover would be cleaner/better.

For time being I just wanted to keep it as close to existing implementation. I looked into passing discover directly as there already is a method balance in channel::mod.rs that does exactly that.
The problem is that method takes Connection which is not public. The code in balance_list and ServiceList which implements Discover converts public Endpoint to private Connection.

If we wanted to expose Discover directly we would need to make Connection public or seek some other solution which converts from Endpoint to Connection.

Ultimately, I am not entirely sure how to solve this conundrum :)

LucioFranco · 2020-05-08T14:35:02Z

@dawid-nowak could we have a (Key, Endpoint) pair? That is sent over the channel via the enum https://docs.rs/tower-discover/0.3.0/tower_discover/enum.Change.html. We can then use endpoint which is public then internally translate it to a connection? That should work right?

dawid-nowak · 2020-05-08T15:07:38Z

I think we are in agreement that ideally that would be the case.

Expose Discover that produces the stream of Change events.
I had a dig around and I think the main obstacle is that tower::Discover trait takes Service type. At the moment Endpoint is not a service so I am not sure how would that work.

Is that correct ?

LucioFranco · 2020-05-08T15:17:18Z

@dawid-nowak yeah, so I think instead of exposing a D: Discover we just assume that the user always wants to configure the tonic clients via Endpoint. This way we can just have a channel::<Change<SomeKey, Endpoint>> we can then pass out the tx side and let the user push change events.

This way we can implement discover ourselves by taking the stream of the channel change events and mapping them to our internal connection type. So internally we would implement a new discover type similar to ServiceList but that wraps a the rx end of a channel of change enum where the value is Endpoint.

Hopefully that makes a bit of sense, I am totally happy to sketch some thing out a bit more :)

dawid-nowak · 2020-05-08T15:23:17Z

yeah, that makes sense,
will need to figure out how to map those streams :) so if you have examples handy that would speed up the process

LucioFranco · 2020-05-08T16:44:17Z

basically I think we can do something similar to this https://docs.rs/tonic/0.2.1/tonic/transport/channel/struct.Channel.html#method.balance_list. Where we have:

fn balance_channel::<K>() -> (Sender<Change<K, Endpoint>>, Self)

This then would allow the user to:

let (tx, channel) = Channel::balance_channel();

tx.send(("some_key", some_endpoint)).await.unwrap();

channel.call(...).await;

We then implement a new discover type that is similar to ServiceList but uses a channel. We can then do something very similar to this https://github.com/hyperium/tonic/blob/master/tonic/src/transport/channel/mod.rs#L115

Take 2 after code review. Changing main interface to take a stream of tower:Change(K,Endpoint) which will get converted to Change:(K,Connection) modified: examples/src/load_balance_with_discovery/client.rs modified: tonic/src/transport/channel/endpoint.rs modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/mod.rs modified: tonic/src/transport/service/discover.rs modified: tonic/src/transport/service/mod.rs

dawid-nowak · 2020-05-11T09:57:29Z

Take 2. I think we are getting closer.

* Converting balance_list to use balance_channel internally. modified: examples/Cargo.toml modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs modified: tonic/src/transport/service/mod.rs

…th_dynamic_discovery Conflicts: tonic/src/transport/service/discover.rs

* keeping fmt happy Changes to be committed: modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs

LucioFranco

Looking great! I suggested a few changes, let me know if you have any questions :)

tonic/src/transport/channel/mod.rs

tonic/src/transport/service/discover.rs

Changes to be committed: modified: examples/src/load_balance_with_discovery/client.rs modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs

* Removing Unpin for K Changes to be committed: modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs

LucioFranco

looks great! left a few comments, I think we are almost there!

LucioFranco · 2020-05-15T14:16:25Z

examples/Cargo.toml

@@ -42,6 +42,14 @@ path = "src/load_balance/client.rs"
 name = "load-balance-server"
 path = "src/load_balance/server.rs"

+[[bin]]
+name = "load-balance-client-discovery"


What do you think about naming this dynamic-load-balance? Might be a bit more clear

LucioFranco · 2020-05-15T14:20:24Z

tonic/src/transport/channel/mod.rs

 use crate::{body::BoxBody, client::GrpcService};
 use bytes::Bytes;
 use http::{
    uri::{InvalidUri, Uri},
    Request, Response,
 };
 use hyper::client::connect::Connection as HyperConnection;
+use std::hash::Hash;


minor nit: can we move this import into the one below?

LucioFranco · 2020-05-15T14:20:57Z

tonic/src/transport/channel/mod.rs

-            .unwrap_or(DEFAULT_BUFFER_SIZE);
+        let (channel, mut tx) = Self::balance_channel(DEFAULT_BUFFER_SIZE);
+        list.for_each(|endpoint| {
+            let _res = tx.try_send(Change::Insert(endpoint.uri.clone(), endpoint));


probably should just unwrap here if there is an error we want to know because that would mean there is a bug in this code!

LucioFranco · 2020-05-15T14:23:36Z

tonic/src/transport/channel/mod.rs

+    /// This creates a [`Channel`] that will listen to a stream of change events and will add or remove provided endpoints.
+    pub fn balance_channel<K>(
+        capacity: usize,
+    ) -> (Self, tokio::sync::mpsc::Sender<Change<K, Endpoint>>)


might want to just import this type, its making this signature a bit long 😄

LucioFranco · 2020-05-15T14:23:58Z

tonic/src/transport/service/discover.rs

@@ -1,34 +1,34 @@
 use super::super::service;
 use super::connection::Connection;
 use crate::transport::Endpoint;
+use std::hash::Hash;


LucioFranco · 2020-05-15T14:24:57Z

tonic/src/transport/service/discover.rs

-        Option<Pin<Box<dyn Future<Output = Result<Connection, crate::Error>> + Send + 'static>>>,
-    i: usize,
+pub(crate) struct DynamicServiceStream<K: Hash + Eq + Clone> {
+    changes: tokio::sync::mpsc::Receiver<Change<K, Endpoint>>,


same here can we import this type?

LucioFranco · 2020-05-15T14:25:48Z

examples/src/load_balance_with_discovery/client.rs

+
+#[tokio::main]
+async fn main() -> Result<(), Box<dyn std::error::Error>> {
+    let e1 = Endpoint::from_static("http://[::1]:50051").timeout(std::time::Duration::from_secs(1));


why the timeout here?

just me playing around, removed

modified: examples/Cargo.toml modified: examples/README.md renamed: examples/src/load_balance_with_discovery/client.rs -> examples/src/dynamic_load_balance/client.rs renamed: examples/src/load_balance_with_discovery/server.rs -> examples/src/dynamic_load_balance/server.rs modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs

dawid-nowak

Updated

LucioFranco

Looks fantastic! Thanks!

dawid-nowak · 2020-05-15T19:05:12Z

🤘

* examples: update to `tracing` 0.1.14, use `#[instrument]` Now that `tracing-attributes`'s `#[instrument]` macro plays nicely with `async-trait`, we can update the tracing example to use `instrument`. This lets us simplify the events emitted in the example. Signed-off-by: Eliza Weisman <[email protected]> * feat(transport): Dynamic load balancing (#341) * Fix typo (#356) Co-authored-by: Dawid Nowak <[email protected]> Co-authored-by: Paulo Duarte <[email protected]>

LucioFranco reviewed May 7, 2020

View reviewed changes

dawid-nowak added 3 commits May 11, 2020 22:13

Load balancing with dynamic discovery

8f8bf4f

* Converting balance_list to use balance_channel internally. modified: examples/Cargo.toml modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs modified: tonic/src/transport/service/mod.rs

Merge remote-tracking branch 'upstream/master' into load_balancing_wi…

f93e030

…th_dynamic_discovery Conflicts: tonic/src/transport/service/discover.rs

Load balancing with dynamic discovery

523ec8f

* keeping fmt happy Changes to be committed: modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs

LucioFranco requested changes May 14, 2020

View reviewed changes

tonic/src/transport/channel/mod.rs Outdated Show resolved Hide resolved

tonic/src/transport/channel/mod.rs Outdated Show resolved Hide resolved

tonic/src/transport/service/discover.rs Outdated Show resolved Hide resolved

dawid-nowak added 2 commits May 14, 2020 21:02

Load balancing with dynamic discovery - core review comments

f69870e

Changes to be committed: modified: examples/src/load_balance_with_discovery/client.rs modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs

Load balancing with dynamic discovery - making fmt happy

8618705

dawid-nowak requested a review from LucioFranco May 14, 2020 21:44

dawid-nowak added 2 commits May 14, 2020 22:49

Load balancing with dynamic discovery - code review comments

078412b

* Removing Unpin for K Changes to be committed: modified: tonic/src/transport/channel/mod.rs modified: tonic/src/transport/service/discover.rs

Load balancing with dynamic discovery - making fmt happy again

821abd9

LucioFranco reviewed May 15, 2020

View reviewed changes

dawid-nowak commented May 15, 2020

View reviewed changes

LucioFranco approved these changes May 15, 2020

View reviewed changes

LucioFranco changed the title ~~Load balancing with dynamic discovery~~ feat(transport): Dynamic load balancing May 15, 2020

LucioFranco merged commit 85ae0a4 into hyperium:master May 15, 2020

dawid-nowak deleted the load_balancing_with_dynamic_discovery branch May 15, 2020 19:14

hawkw pushed a commit to hawkw/tonic that referenced this pull request Jul 10, 2020

feat(transport): Dynamic load balancing (hyperium#341)

808ecff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(transport): Dynamic load balancing #341

feat(transport): Dynamic load balancing #341

dawid-nowak commented May 5, 2020

LucioFranco May 7, 2020

LucioFranco commented May 7, 2020

dawid-nowak commented May 8, 2020

LucioFranco commented May 8, 2020

dawid-nowak commented May 8, 2020

LucioFranco commented May 8, 2020

dawid-nowak commented May 8, 2020

LucioFranco commented May 8, 2020

dawid-nowak commented May 11, 2020

LucioFranco left a comment

LucioFranco left a comment

LucioFranco May 15, 2020

dawid-nowak May 15, 2020

LucioFranco May 15, 2020

dawid-nowak May 15, 2020

LucioFranco May 15, 2020

dawid-nowak May 15, 2020

LucioFranco May 15, 2020

dawid-nowak May 15, 2020

LucioFranco May 15, 2020

dawid-nowak May 15, 2020

LucioFranco May 15, 2020

dawid-nowak May 15, 2020

LucioFranco May 15, 2020

dawid-nowak May 15, 2020

dawid-nowak left a comment

LucioFranco left a comment

dawid-nowak commented May 15, 2020

feat(transport): Dynamic load balancing #341

feat(transport): Dynamic load balancing #341

Conversation

dawid-nowak commented May 5, 2020

Motivation

Solution

Choose a reason for hiding this comment

LucioFranco commented May 7, 2020

dawid-nowak commented May 8, 2020

LucioFranco commented May 8, 2020

dawid-nowak commented May 8, 2020

LucioFranco commented May 8, 2020

dawid-nowak commented May 8, 2020

LucioFranco commented May 8, 2020

dawid-nowak commented May 11, 2020

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco left a 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

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

dawid-nowak left a comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

dawid-nowak commented May 15, 2020