Support Elasticsearch RestClientBuilder auto-configuration without RestHighLevelClient

[filiphr]

Prior to this commit, Spring Boot would only auto-configure
the RestHighLevelClient and RestClientBuilder if the RestHighLevelClient
was present. This was done in 1d73d4e as part of #22358

This commit brings back the exposing of the RestClient bean in
Spring Boot when exposing the RestHighLevelClient or
when the RestHighLevelClient is not present.
It allows for using the Spring Boot auto configuration and its customizers
of the RestClientBuilder in a similar way as it is done for the RestTeamplateBuilder
and the WebClient.Builder.
Now the presence of the org.elasticsearch.client:elasticsearch-rest-high-level-client is optional.
This opens the door for potentially adding support to the new
Elasticsearch Java Client that is based on the same RestClient

---

The exposing of the RestClient as a bean is not the most important thing in this PR.
I would rather say that the fact that the RestClientBuilder is exposed as a bean is the most important part.
This allows users to rely on the Spring Boot provided mechanisms for configuring the RestClientBuilder and not depend on the now deprecated Elasticsearch Rest High Level Client.

[filiphr]

I didn't do any changes to the Gradle Wrapper. Seems like some problem in the Gradle Wrapper action itself

[bclozel]

This PR is reversing the team's decision explained in #22358 (comment). I'm not sure I understand the rationale behind this. The presence of elasticsearch-rest-high-level-client has always been optional, but we've found that the low level client was of little use in itself at the application level.

If the main goal is to support the newly released client, maybe we should auto-configure as well and deprecate the high-level variant in 2.6.x?

[filiphr]

The biggest reason why I created this PR was to get the RestClientBuilder as a bean in order to rely on the Spring properties / customizers. With the current setup of things you have to have elasticsearch-rest-high-level-client in order to create your own RestClient.

The reason for that is

spring-boot/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfiguration.java

Lines 41 to 42 in 83e4430

	@ConditionalOnClass(RestHighLevelClient.class)
	@ConditionalOnMissingBean(RestClient.class)

We have a platform that heavily uses Spring Boot and we are using the RestClient directly and not the RestHighLevelClient. We started removing the dependency entirely, since we want to completely not depend on it due to Licensing issues. The High Level Client is since 7.11 no longer Apache 2.0 licensed. I thought that everything will keep working as expected, but then the RestClientBuilder was not present anymore.

The addition of the RestClient as a bean is an addition to the original requirement that we had. I honestly, thought that it is not that complicated to also expose it as a bean since it can be exposed through the RestHighLevelClient#getLowLevelClient easily and if the RestHighLevelClient is not there then it can easily be created through the RestClientBuilder. That's why I did it.

Even if you only partially integrate this (without the new beans) we can easily do

@Bean
public RestClient elasticsearchRestClient(RestTemplateBuilder builder) {
    return builder.build();
}

and have the RestClient bean in our own application without the need to depend on the RestHighLevelClient and without the need to copy things that are already done in Spring Boot.

---

In addition to all of this, there is also OpenSearch with its own things. Without going into too much detail, for our own simple purposes it might be possible to use one RestClient to communicate with both types of clusters.

---

In a nutshell for us having to maintain support for our own things on both OpenSearch and Elasticsearch it makes a lot of sense to only use the Low Level Clients. I hope that you are going to take this into consideration and revise your decision about this. Thanks a lot for the understanding

[bclozel]

Thanks for the background information @filiphr, that's super useful.

With that I think we should:

• bring back RestClient or its builder as a bean. This should allow the OpenSearch use case
• schedule the support for the new official client and deprecate the current one

[filiphr]

Something else, that I forgot to add to this PR. The ElasticSearchRestHealthContributorAutoConfiguration needs to be adapted as well.

Ideally the health contributor will be adapted to only use RestClient or somehow a combination of things (if the RestHighLevelClient is there). e.g. All RestClient beans plus the RestHighLevelClient#getRestLowLevelClient() that are not exposed as beans. The ElasticsearchRestHealthIndicator needs to be adapted to only take the RestClient as a constructor in order to not have a mandatory dependency on the RestHighLevelClient. If this is done then the same class can be used both for the old RestHighLevelClient and new Elasticsearch Java Client.

I can of course provide PRs and / or adapt this PR if you decide that you are OK with my proposals.

[filiphr]

One, I hope final, argument is also the fact that removing the dependency on the RestHighLevelClient just shaved off ~20MB from our final jar. Not sure if things like this are important from your perspective or not, but I wanted to share this as well

[bclozel]

Thanks a lot @filiphr for all your insights.

We've just discussed this as a team and we've decided to:

1. Bring back RestClient as a bean; we initially decided that it had low value for actual applications, but with the latest changes in the elasticsearch ecosystem, it's better to have it.
2. Adapt the health contributor to only depend on the low level RestClient
3. Deprecate the RestHighLevelClient in 2.7, see Deprecate Elasticsearch RestHighLevelClient #28598
4. Support the new ElasticsearchClient in 2.7 as a replacement, see Support new ElasticsearchClient #28597

Could you adapt this PR to address 1) and 2)?
Note that we've scheduled that for 2.7, so there's no rush on this as we're going to focus on getting 2.6 out and critical bug fixes first.

[filiphr]

Thanks a lot for this decision @bclozel.

I am going to adapt this PR to take into consideration for 1) and 2).

What about exposing RestClientBuilder as a bean when the RestHighLevelClient is not present, but the RestClientBuilder is. Will you accept a separate PR for 2.6 that will do this?

I have managed to achieve what I need with 2.5 already, but the way I did it is not the best approach, since I am doing some nasty magic with ImportSelector, and I would rather get rid of that nasty magic we have.

[bclozel]

What about exposing RestClientBuilder as a bean when the RestHighLevelClient is not present, but the RestClientBuilder is. Will you accept a separate PR for 2.6 that will do this?

Exposing the RestClientBuilder might work for your case, but I'm wondering if it's the right choice here: I don't think we usually want to have several instances of RestClient in the same application? That would create several HTTP client instances, right? Even if the application itself only creates one, we'd need to reuse it for the health indicator?

It's a bit late in the 2.6 cycle unfortunately so if we want to introduce a change here, it has to be extra-safe.

[filiphr]

@bclozel I think that I have adapted the PR as you asked. Please have a look at it and let me know if there is something more that needs to be done.

Also adapted the use of ElasticsearchRestHealthIndicator. I deprecated the constructor using RestHighLevelClient. Not sure what your policy for removal there is. Ideally I'd like to get rid of the constructor, since the ElasticsearchRestHealthIndicator in the current state is not usable when the RestHighLevelClient is not there. I think that the class will not load at all.

---

Exposing the RestClientBuilder might work for your case, but I'm wondering if it's the right choice here: I don't think we usually want to have several instances of RestClient in the same application? That would create several HTTP client instances, right? Even if the application itself only creates one, we'd need to reuse it for the health indicator?

Exposing the RestClientBuilder does not mean that there will be several instances of the RestClient. The build method needs to be invoked for a RestClient to be created. Which means that from a Spring Boot point of view for 2.6 when the RestHighLevelClient is not there then there will be a bean of type RestClientBuilder. The users can then choose and decide what to do with that RestClientBuilder. In order to reduce the amount of changes the health indicator will not use it. People will need to expose their own health indicator in 2.6. And as I mentioned in the previous paragraph the current ElasticsearchRestHealthIndicator is not capable to be used without the RestHighLevelClient.

[bclozel]

@filiphr you're right, I forgot we were already exposing the builder as a bean. I need to take a deeper look.

[filiphr]

@bclozel I'll try to do a small proposal as a separate PR for that. You can then decide what you want to do

[filiphr]

@bclozel I did a proposal in PR #28655. Please have a look at it and let me know what you think. This PR and that one of course have some overlap. However, based on your decision, I can adapt this PR on top of that other one if needed.

[filiphr]

@bclozel I finally found the time and adapted the changed in this PR to address point 1) and 2) from #28496 (comment). I've changed the base of the PR to be 2.7. Please have a look at it and let me know what you think about it.

[snicoll]

For the record 2.7.x has been upgraded to Elasticsearch 7.16.2.

[filiphr]

For the record 2.7.x has been upgraded to Elasticsearch 7.16.2.

Damn, I first rebased on main, but then realized that main and 2.7.x are not the same, so I did it on 2.7.x. I'll try to do take the new changes from 2.7.x into consideration and fix the conflicts.

Basically, I need to make sure that there are no deprecation warnings, right?

[wilkinsona]

Thanks very much, @filiphr. If you're interested, I reworked things a bit in a7a71da. This was primarily to simplify the changes to the health indicator. You can see the net changes in 227c316.

[filiphr]

@wilkinsona I agree with you that the code is simplified with your change. However, your change means that you can only have Elasticsearch health checks if you have the RestHighLevelClient on the classpath, because the loading of ElasticsearchRestHealthIndicator will fail due to the RestHighLevelClient being used in one of its constructors.

I personally see no need to lose Elasticsearch auto configuration if you do not have RestHighLevelClient on the classpath. Due to the changes in ElasticSearchRestHealthContributorAutoConfiguration it also means that the application will not boot up properly when you only have the RestClient available.

If this is something that the Boot team wants to do, then I am all OK with it. However, I would ask you to reconsider and have health checks without the RestHighLevelClient.

[wilkinsona]

That’s an unintentional over-simplification. Thanks for catching it, @filiphr. I’ll take another look. Requiring a RestClient bean for auto-configuration of the health indicator looks like it could be a good middle ground.

[filiphr]

Requiring a RestClient bean for auto-configuration of the health indicator looks like it could be a good middle ground.

It is not about the auto-configuration. The auto configuration works correctly and it auto configures if a RestClient bean is there. The problem is the fact that the loading of the ElasticsearchRestHealthIndicator will fail when the RestHighLevelClient class is not there.

Thanks for taking another look at it.

[filiphr]

Thanks a lot for bringing back the old changes @wilkinsona. It is really appreciated.

[nightswimmings]

@wilkinsona Are we opensearch users expected to keep using opensearch.RestHighLevelClient after 2.7.0?

[wilkinsona]

Spring Boot does not have any auto-configuration for OpenSearch so we don't have any expectations about what you should do.

[filiphr]

@nightswimmings as far as I know, you can keep using the Elasticsearch RestClient with OpenSearch. You can't use the RestHighLevelClient though.

[nightswimmings]

Thanks filiphr, we already have all forked opensearch client code, its just that I did not know whether to expect any impact or not on the new autconfiguration and deprecation if you are on opensearch-flavoured els, but if Boot does not account the later, then its not an issue