elastic / elasticsearch-rs

Official Elasticsearch Rust Client
https://www.elastic.co/guide/en/elasticsearch/client/rust-api/current/index.html
Apache License 2.0
705 stars 72 forks source link

Implement Multinode connection pool #139

Closed srleyva closed 1 month ago

srleyva commented 4 years ago

57

srleyva commented 4 years ago

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?

russcam commented 4 years ago

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.

srleyva commented 4 years ago

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. :)

russcam commented 4 years ago

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

elasticmachine commented 4 years ago

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?

srleyva commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 😄

russcam commented 3 years ago

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

mfelsche commented 2 years ago

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?

tommilligan commented 2 years ago

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.

mfelsche commented 2 years ago

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

tommilligan commented 2 years ago

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

swallez commented 1 month ago

Superseded by PR #189