Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Elasticsearch RestClient auto-configuration #22358

Closed
latuszek opened this issue Jul 16, 2020 · 6 comments
Closed

Remove Elasticsearch RestClient auto-configuration #22358

latuszek opened this issue Jul 16, 2020 · 6 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@latuszek
Copy link

spring-boot-starter-parent v2.3.1.RELEASE
elasticsearch-rest-high-level-client (version inherited from parent above)

According to spring-boot documentation:

If you have the org.elasticsearch.client:elasticsearch-rest-high-level-client dependency on the classpath, Spring Boot will auto-configure a RestHighLevelClient, which wraps any existing RestClient bean, reusing its HTTP configuration.

In our project, we have a single RestClient bean registered in spring context:

    @Bean
    public RestClient elasticsearchRestClient(ElasticsearchProperties elasticsearchProperties) {
        RestClientBuilder builder = RestClient.builder(
                new HttpHost(elasticsearchProperties.getHost(),
                        elasticsearchProperties.getPort(),
                        elasticsearchProperties.getScheme())
        );

        ElasticsearchCredentials esCredentials = new ElasticsearchCredentials(elasticsearchProperties);
        if (esCredentials.hasCredentials()) {
            builder.setHttpClientConfigCallback(httpClientBuilder -> httpClientBuilder.setDefaultCredentialsProvider(
                    esCredentials.getCredentialsProvider()));
        }

        return builder.build();
    }

Spring actuator health endpoint returns correct response regarding connection to Elasticsearch (uses our parameters etc.).
However when we're trying to use injected RestHighLevelClient to connect (e.g. using client.cluster().health(new ClusterHealthRequest(), RequestOptions.DEFAULT); it connects using default parameters from spring-boot.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 16, 2020
@snicoll
Copy link
Member

snicoll commented Jul 16, 2020

Thanks for the report. I can see the documentation does not match what the auto-configuration does as far as I can see and I am not sure I understand why. I don't see a way to create a RestHighLevelClient based on an existing RestClient so perhaps something was changed in ElasticSearch and the documentation wasn't updated?

In any case, we should not create the high-level client if a RestClient exists I guess.

As for the sample above, You should not inject ElasticSearchProperties in your own code but rather use RestClientBuilderCustomizer for whatever customization that you need to do on the builder. This will fix this issue as a side-effect of it.

I've flagged this issue for team attention to see if someone else on the team can fill the blanks.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Jul 16, 2020
@bclozel
Copy link
Member

bclozel commented Jul 16, 2020

I guess this is mainly a documentation issue.

It should be instead:

You can also register an arbitrary number of beans that implement RestClientBuilderCustomizer for more advanced customizations. To take full control over the registration, define a RestClientBuilder bean.

If you have the org.elasticsearch.client:elasticsearch-rest-high-level-client dependency on the classpath, Spring Boot will auto-configure a RestHighLevelClient, which leverages any existing RestClientBuilder bean, reusing its HTTP configuration.

In your case @latuszek, your bean method could be returning a RestClientBuilder and everything should work as expected.

@latuszek
Copy link
Author

Thanks @bclozel.

@philwebb philwebb added type: documentation A documentation update and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jul 20, 2020
@philwebb philwebb added this to the 2.3.x milestone Jul 20, 2020
@bclozel bclozel changed the title Elasticsearch RestHighLevelClient does not inject existing (custom) RestClient Fix docs on Elasticsearch Rest client customization Aug 7, 2020
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Aug 11, 2020
@snicoll
Copy link
Member

snicoll commented Aug 11, 2020

While this isn't possible to create a RestHighLevelClient from a RestClient, there is a protected method that's explicitly documented as a way to do this. Unfortunately, we already have a soft link between the RestHighLevelClient and the RestClient and creating one in the other direction leads to a BeanCurrentlyInCreationException.

With that taken into consideration and what the code currently does, it would be nicer if we didn't configure a RestHighLevelClient if a RestClient is provided as the chances for it to be out-of-sync is quite high. Also, you'd usually configure the high-level client and just access the low-level RestClient if you need that rather than the other way around.

We could probably add a warning in the documentation about this use case and change the behaviour in the next feature release if we think that makes sense. I've flagged this for team attention.

snicoll added a commit to snicoll/spring-boot that referenced this issue Aug 11, 2020
@philwebb philwebb self-assigned this Aug 12, 2020
@bclozel
Copy link
Member

bclozel commented Aug 24, 2020

I agree with @snicoll here.

RestClient operates directly at the HTTP level and seems to be useful for infrastructure purposes or very specific use cases. It doesn't handle deserialization and requires you to deal with the HTTP request/response directly in many cases. RestHighLevelClient seems more suited for Spring applications.

RestClient is still a good choice for our own infrastructure (like actuators), but I'm wondering if we should expose it as a bean at all as it makes things more confusing here. On the other hand, RestClient and RestHighLevelClient don't ship with the same JAR: elasticsearch-rest-high-level-client vs elasticsearch-rest-client. This doesn't make our choice here easier.

@philwebb philwebb modified the milestones: 2.3.x, 2.4.x Aug 24, 2020
@philwebb philwebb added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review type: documentation A documentation update labels Aug 24, 2020
@philwebb philwebb assigned bclozel and unassigned philwebb Aug 24, 2020
@bclozel
Copy link
Member

bclozel commented Aug 25, 2020

After discussing this with the team, we've decided the following:

  • we won't auto-configure a RestClient anymore, only RestHighLevelClient is supported, making the elasticsearch-rest-high-level-client dependency a requirement
  • applications can still get the RestClient from the auto-configured RestHighLevelClient if needed
  • if the application has very specific needs and only wants to depend on elasticsearch-rest-client, we consider that the use case is specific enough and this should be dealt with at the application level

@bclozel bclozel changed the title Fix docs on Elasticsearch Rest client customization Remove Elasticsearch RestClient auto-configuration Aug 25, 2020
@snicoll snicoll self-assigned this Sep 8, 2020
@snicoll snicoll modified the milestones: 2.4.x, 2.4.0-M3 Sep 8, 2020
@bclozel bclozel closed this as completed in 1d73d4e Sep 8, 2020
bclozel added a commit that referenced this issue Sep 8, 2020
snicoll added a commit that referenced this issue Sep 8, 2020
This commit makes sure that no high-level client is auto-configured if
a low-level client is registered as a bean.

See gh-22358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants