-
Notifications
You must be signed in to change notification settings - Fork 41.2k
Add socketKeepAlive configuration property for Elasticsearch #32051
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
Conversation
@puppylpg Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@puppylpg Thank you for signing the Contributor License Agreement! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposal, @puppylpg. I've left a couple of comments for you consideration. In addition to those, it would be good to have a test that verifies that our default for the new property aligns with Elasticsearch's default. If the Elastic default is changed in the future, this will ensure that we notice and can update our default to match.
.../main/java/org/springframework/boot/autoconfigure/elasticsearch/ElasticsearchProperties.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. I've left a comment about the testing of Elastic's default.
...ramework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
@puppylpg thank you for making your first contribution to Spring Boot. |
Will this mr be merged into 2.x.x besides 3.0.0-M5? |
Sorry, it won't. It's an enhancement so it only belongs in a new major or minor release. We don't have any plans at the moment for a 2.8 release so 3.0 is the next available release in which it can be included. |
ok, thanks for your reply (sad, for still using jdk8 in our production environment 😅😅😅) |
Keepalive property for the connection between client and elasticsearch should be considered as configurable. Futher more, i do think it's good to keep it alive by default.
Here is the issue:
When read/write elasticsearch in high concurrency, threads have chances to stuck on obsolete connection between client and elasticsearch. The more threads involves, the quicker stucking on connection happens. The reason is described here:
And here is the situation i run into. Logs can also be found here:
similar issue:
However, spring boot can't config keepalive through current properties. We have to write some config codes like:
So i add some support for
spring.elasticsearch.keepalive=true
to make it easier to config.