elasticsearch-rs icon indicating copy to clipboard operation
elasticsearch-rs copied to clipboard

Implement Multinode connection pool

Open srleyva opened this issue 3 years ago • 13 comments

  • This commit adds a connection pool that takes a static list of nodes and distributes the load.

  • trait for setting connection distribution. Defaults to RoundRobin.

#57

srleyva avatar Sep 13 '20 18:09 srleyva

I believe your assessment is correct as far as changing the ConnectionPool interface to account for interior mutability. It seems the only way to do that across threads is a mutex type. I think there’s overhead in requiring every ConnectionPool type to require a Mutex even if it’s not dynamic but it’s only way I see how to do this. Any thoughts?

srleyva avatar Sep 16 '20 06:09 srleyva

Thinking about sniffing further and discussing with folks on the clients team, the connection pool is responsible for managing one or more connections (connections being details about nodes in the cluster such as Uri) and data related to connections such as whether the pool supports sniffing, but the Transport is responsible for acting on this data i.e. the Transport should perform sniffing and (re)seed the pool if the pool supports it.

With this in mind, a sniffing connection pool would likely have a Arc<RwLock<Vec<Connection>>> (or similar construct) to allow multi-threaded reads through immutable references, with a single writer through a mutable reference. A question that I have is how this kind of connection pool advertises itself e.g. through the ConnectionPool trait with something like

pub trait ConnectionPool: Debug + dyn_clone::DynClone + Sync + Send {
    /// Gets a reference to the next [Connection]
    fn next(&self) -> &Connection;

    fn reseedable(&self) -> bool {
        false
    }

    fn reseed(&self, connections: Vec<Connection>) {}
}

Would need to play around to get the right type and function design for this. StaticNodeListConnectionPool might then be more aptly named MultiNodeConnectionPool with functions to construct sniffable and non-sniffable i.e. static versions.

russcam avatar Sep 21 '20 01:09 russcam

WIP as I still need to implement the NodeInfo parts, but thought I'd get feedback on the current reseed implementation and the changes to the ConnectionPool trait. :)

srleyva avatar Sep 21 '20 06:09 srleyva

Really appreciate your effort, @srleyva! I've left some comments

russcam avatar Sep 21 '20 07:09 russcam

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine avatar Sep 21 '20 20:09 elasticmachine

First pass at node sniff request following elastic docs. I thought even if aren't sure of the final design, having the request parts up would be easier.

Manual tests for round robin load balancing on a local docker-compose ES cluster look quite promising when seeded with http://localhost:9200 :smile:

Ping Response: 200 OK Url: http://172.23.0.3:9200/
Ping Response: 200 OK Url: http://172.23.0.6:9200/
Ping Response: 200 OK Url: http://172.23.0.5:9200/
Ping Response: 200 OK Url: http://172.23.0.2:9200/
Ping Response: 200 OK Url: http://172.23.0.3:9200/
Ping Response: 200 OK Url: http://172.23.0.6:9200/
Ping Response: 200 OK Url: http://172.23.0.5:9200/
Ping Response: 200 OK Url: http://172.23.0.2:9200/
Ping Response: 200 OK Url: http://172.23.0.3:9200/

srleyva avatar Sep 23 '20 03:09 srleyva

I'll play around with using a background thread to reseed over the next few days. Thanks for your patience and guidance on this as I am still very new to rust. :smile:

srleyva avatar Sep 24 '20 06:09 srleyva

Hey @russcam is there any update on this PR? Apologies for the lack of communication. Its feature complete according to the original requirements. If there's anything else that's needed please let me know 😄

srleyva avatar Jan 18 '21 23:01 srleyva

Thanks for the ping, @srleyva 👍 Hoping to get a chance to look at this again this week, unless @swallez beats me to it!

russcam avatar Jan 27 '21 01:01 russcam

What is holding this PR back? We would like to use the official client, but without at least a way to target multiple elastic nodes, this is not feasible for us. Is there any way I could help out to get this over the finish line?

mfelsche avatar Nov 18 '21 14:11 mfelsche

I'm also interested in getting this merged. The branch looks stale and based on old dependencies - I'll rebase it on master and update to newer dependencies, and open as a new PR.

tommilligan avatar Nov 19 '21 08:11 tommilligan

Great! @tommilligan ping me, if you need another hand or pair of eyes. :)

mfelsche avatar Nov 19 '21 08:11 mfelsche

@mfelsche I've opened the rebased PR at https://github.com/elastic/elasticsearch-rs/pull/189

tommilligan avatar Nov 22 '21 16:11 tommilligan