`Rest5ClientBuilder` should have a callback to configure the `CloseableHttpAsyncClient`

[sothawo]

Description

I am currently migrating the next version of Spring Data Elasticsearch to use the new Rest5Client and have a light problem:

The old RestClientBuilder was offering a HttpClientConfigCallback which Spring Data Elasticsearch used to configure many aspects of the http client (proxy, ssl stuff, ...).

In the new Rest5ClientBuilder there is no such way to configure the CloseableHttpAsyncClient that is created. This means that I would have to create my own client from scratch?

Is it planned to add such a callback again? I'd rather build on a basic http client provided by the Rest5ClientBuilder than creating my own.

[l-trotta]

Hey! The reason why we removed the callback method is because basically it proxies the HttpAsyncClientBuilder from the apache client, and the new builder in version 5 works a bit differently, delegating some of the connection settings to an instance of AsyncClientConnectionManager, so we couldn't replicate exactly what we had before for every setting (ssl for example). Since the builder wouldn't looks the same, we decided we'd try to simplify things for users by providing dedicated setters for the most common configurations, for example you can set SSL by just calling the setter:

Rest5ClientBuilder builder = Rest5Client.builder(
    new HttpHost("localhost", 9200))
    .setSSLContext(SSLContext.getDefault());

Still, I can see that there are many other configuration options that we didn't account for, so we could bring back HttpClientConfigCallback, but it would be only for options that are directly accessible from the HttpAsyncClientBuilder, and not the AsyncClientConnectionManager because otherwise it would override our default connection settings. Would it be acceptable?

[sothawo]

Makes totally sense, I managed to work my way through configuring; I used to expose the configuration callback to Spring Data Elasticsearch users, but I don't see a problem with not having this anymore, or maybe be able to re-add later. Our next release will be a major release update and so will have some changes for our users, some breaking.

So I'll close this issue.

[l-trotta]

Still deciding on whether to re-add it or not, the main problem is that it would allow users to overwrite our ConnectionManager configuration which has the default timeouts optimized for the Elasticsearch server. In the meantime, I merged #1019 to add dedicated setters for proxy settings.

[wilkinsona]

I've been affected by this when moving Spring Boot from RestClient to Rest5Client.

With RestClient, we could fine-tune the HTTP client builder using HttpClientConfigCallback or the lower-level request config using RequestConfigCallback. This really helped in situations where the defaults were largely wanted with only one or two tweaks. With Rest5Client, it's now harder to fine-tune one or two settings while keeping the defaults for the others.

Still deciding on whether to re-add it or not, the main problem is that it would allow users to overwrite our ConnectionManager configuration

I think the current lack of callbacks makes this more likely, not less. In many cases, it's now necessary to provide an entire HTTP client in order to change a single setting. Unless you look at the Rest5ClientBuilder source and carefully copy over its defaults, you're like to inadvertently change something.

Providing callbacks for the various builders that are involved in creating the HTTP client would help to avoid this problem. Unfortunately, due to changes in the underlying Apache HTTP client, there are now more of them, but I still think it might be worthwhile to expose them:

• HttpAsyncClientBuilder
• RequestConfig.Builder
• PoolingAsyncClientConnectionManagerBuilder
• ConnectionConfig.Builder

Our migration from RestClient to Rest5Client is still a work in progress. If you're interested, you can find the changes in this branch. The removal of the callbacks has been felt most acutely in this class where we're offering the same configurability that we had with RestClient while also trying to preserve the Rest5ClientBuilder defaults.

[l-trotta]

Hello @sothawo, @wilkinsona, after discussing internally we have decided to reintroduce the callback to allow complete configuration of the HttpAsyncClientBuilder, and also keeping the option to provide an already built instance of HttpAsyncClient. A new version of the client will be released ASAP, I'll update as soon as it's available to download.

We hope it makes it easier to update to the new Rest5Client, the basic idea was providing less configuration options since in most cases the defaults work fine, and trying to avoid overriding such default values, but we understand that for more advanced use cases it makes it hard to configure basic options.

[l-trotta]

see #1037

[wilkinsona]

That's great to hear. Thanks very much, @l-trotta.

[l-trotta]

New version 9.0.4 now available! (not yet visible in maven central) @wilkinsona