-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Extend async search keep alive #67877
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
a144a06
to
47e13b1
Compare
77b21fd
to
15f046d
Compare
Pinging @elastic/es-search (Team:Search) |
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.
I left two minor comments regarding naming but the change looks great, thanks @dnhatn !
x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchTask.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/async/AsyncTaskIndexService.java
Outdated
Show resolved
Hide resolved
Thanks Jim! |
There can be a race between two GET async search requests, and the one with a lower keep_alive parameter wins the race. This scenario is not desirable as we should retain the search result for all requests. This commit ensures the keep_alive is extended and never goes backward.
@jimczi I thought we ended up agreeing that following elastic/kibana#89570 we actually don't need this restriction. |
Good call @lizozom , we added the restriction before the design change in elastic/kibana#89570 but +1 to revert. I'll open a new PR. |
This reverts commit 244fc95.
Reverted by #67877 |
There can be a race between two GET async search requests, and the one with a lower keep_alive parameter wins the race. This scenario is not desirable as we should retain the search result for all requests. This commit ensures the keep_alive is extended and never goes backward.
Relates elastic/kibana#88599