feature: Add multi-node connection pool

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

feature: Add multi-node connection pool #189

Closed tommilligan closed 1 month ago

tommilligan commented 2 years ago

Closes #57 Based on/supersedes #139 by @srleyva

Initially reviewed by @russcam and @swallez

This PR rebases #139 on top of current master, updates the added dependencies to current versions, and fixes clippy/deprecation warnings present in the diff.

I have maintained the original commit history for ease of picking up review from the existing PR - I believe everything after and including Add regex parsing bound_address to URL is unreviewed.

I have a couple of thoughts of potential improvements, but would like to hear from others before I sink any more time into this.

elasticmachine commented 2 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?

Licenser commented 2 years ago

From the perspective of a user of the library, I'm a bit concerned over the number of unwrap() calls in the added logic. Would it be possible to replace them with some kind of handle error?

A library that uwraps/panics is really hard to deal with when it comes to providing helpful errors to users, using Result and some kind of Error allows the application to provide detail, hints, and additional help to a user as to what went wrong also it can then handle a failure gracefully - after all a hard stop of an application is not always the desired behavior on a failure in a library.