feat: Cache ballista clients on executor - #1578
Conversation
03513f4 to
fc5ac1a
Compare
milenkovicm
changed the title
0df68d7 to
e5ec6ee
Compare
milenkovicm
changed the title
e5ec6ee to
6b2e375
Compare
milenkovicm
changed the title
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Really like the direction here. Caching shuffle clients should make a big difference for clusters that hit a lot of executors. Left a few inline thoughts.
milenkovicm
changed the title
|
@martin-g @danielhumanmod @sandugood it would be great if you could review this change. thanks a lot |
| /// creates a ballista client to be used for testing | ||
| /// it connects lazily and which can not really | ||
| /// be reconfigured. | ||
| pub fn new_for_test(host: &str, port: u16) -> Self { |
There was a problem hiding this comment.
There is no #[cfg(test)].
Does it need to be here ? It could be a helper method in mod tests too
There was a problem hiding this comment.
It should but apparently it does not work across different crates, or I'm doing something wrong
| }); | ||
|
|
||
| let weak: Weak<Inner> = Arc::downgrade(&inner); | ||
| // TODO: do we limit minimum interval here? |
There was a problem hiding this comment.
Since we add this in the user-facing config:
If the user sets the client_ttl in the (1, 45) range it will be transformed into 15. Maybe add a warning when user sets this value in the config and it is in the (1, 45) range?
There was a problem hiding this comment.
maybe it would make sense to have warning for ttl < 15 but then there would be a bit of explaining needed.
i was thinking to add it to cli help, but this is implementation specific as well.
I'd propose to keep it as it is and add it later if needed
|
Thanks @milenkovicm. I will try and start reviewing this one tonight. |
7477071 to
85b99b8
Compare
| config: &GrpcClientConfig, | ||
| customize_endpoint: Option<Arc<BallistaConfigGrpcEndpoint>>, | ||
| ) -> Result<PooledClient> { | ||
| let key = (host.to_string(), port, config.clone()); |
There was a problem hiding this comment.
key is missing customize_endpoint?
There was a problem hiding this comment.
Customise endpoint is compile time, it should not change. I propose it ignore it for now
5 participants
Which issue does this PR close?
Closes #736.
Rationale for this change
As mentioned in #1388 (comment), we might run out of available ports to establish new remote connections. Furthermore, connection establishment can sometimes be costly, such as during a TLS handshake. Therefore, we need a mechanism to cache established connections.
What changes are included in this PR?
BallistaClientPoolallows for different client caching strategies.DefaultBallistaClientPoolimplementationDefaultBallistaClientPooldoes not use GRPC channel "clone and use" as it might be harder to configure property, hence pool may have one or more cached connections for same endpoint.Are there any user-facing changes?
Change is NOT backward compatible for implementors of custom executors.
At the moment client pool is disabled, executors behave same like before. To enable it executors should be started with
--connection-cacheoptionAfter trail period we will make caching default.
TODO: