Skip to content

The default value of refresh policy for reactive es template should be null or false #2110

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

Closed
ToffeeLu opened this issue Mar 7, 2022 · 3 comments · Fixed by #2124
Closed
Labels
type: enhancement A general enhancement

Comments

@ToffeeLu
Copy link

ToffeeLu commented Mar 7, 2022

It cost us 1 month to troubleshoot the issue of high indexing latency(p95 10s+) during high traffic with customized routing.
Finally we find that RefreshPolicy is set to IMMEDIATE by default, and latency becomes normal after we manually set it to None.

According to the official document, the default refresh behavior should be set to false unless you're really care about "data consistency". And only Reactive Template has such default value, all others are null.

In order to help save efforts for all users in future, I'd suggest change below two default values to NONE (Not sure if there's any more places):

  1. https://github.com/spring-projects/spring-data-elasticsearch/blob/main/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java#L123
  2. https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/elasticsearch/ElasticsearchDataConfiguration.java#L111
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 7, 2022
@sothawo
Copy link
Collaborator

sothawo commented Mar 8, 2022

You are right, but this is a breaking change in behaviour, so we can do this with the next major release - not speaking for Spring Boot, you'll have to raise an issue there.

@ToffeeLu
Copy link
Author

ToffeeLu commented Mar 8, 2022

Thanks @sothawo
I've post the issue in spring boot project, spring-projects/spring-boot#30096

Also before you guys make this breaking change, I'm thinking is it possible to add this "Configuration" information into the document? Because it's really hard to find it out. It won't cause any error, only big performance loss under high indexing traffic.

@sothawo
Copy link
Collaborator

sothawo commented Mar 23, 2022

I just had a deeper look into this on the Spring Data Elasticsearch side.

When not using Spring Boot, but the configuration setup like it is described in https://docs.spring.io/spring-data/elasticsearch/docs/current/reference/html/#elasticsearch.clients.reactive, the refresh policy will be set to null because in org.springframework.data.elasticsearch.config.AbstractReactiveElasticsearchConfiguration the default value returned by org.springframework.data.elasticsearch.config.AbstractReactiveElasticsearchConfiguration#refreshPolicy is null. This method is used to initialize the template in this configuration approach.

Spring Boot creates the ReactiveElasticsearchTemplate directly with the default initialization in there and so having the refresh set to immediately.

The Spring Data Elasticsearch documentation does not show the direct creation of a ReactiveElasticsearchTemplate, where we could put the hint on this default value.

I have no problems in changing the default value to null, as this would then be the default behaviour of Elasticsearch, but this change would affect user that have the client set up by Spring Boot and not by using the Spring Data Elasticsearch configuration class.

sothawo added a commit to sothawo/spring-data-elasticsearch that referenced this issue Mar 24, 2022
sothawo added a commit that referenced this issue Mar 24, 2022
@sothawo sothawo added this to the 4.4 RC1 (2021.2.0) milestone Mar 24, 2022
sothawo added a commit that referenced this issue Mar 24, 2022
Original Pull Request #2124
Closes #2110

(cherry picked from commit acd7990)
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

Successfully merging a pull request may close this issue.

3 participants