-
Notifications
You must be signed in to change notification settings - Fork 171
Expand Elasticsearch Search timeout #3531
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
base: main
Are you sure you want to change the base?
Conversation
🔍 Preview links for changed docs |
|
Hey @stefnestor thanks for opening this, might be a bit tardy in reviewing this given 9.2 goes out this week. Please re-ping next week if don't hear anything back ;) But in the meantime, perhaps @jimczi could weigh in on these suggested changes, and/or recommend one of the relevant dev teams have a look? |
|
Couple of things off top of my head:
|
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.
Couple linking things :)
|
Thanks, @leemthompo ! I have updated per feedback all points except replying here to: "Why not? 😄" Because it's not defined anywhere, Dev would need to write a doc PR. |
|
Thanks! There's some language cleanup to be done here, but I'd like to get a technical review before doing that. |
| * [`fetch` phase](elasticsearch://reference/elasticsearch/rest-apis/search-profile.md#profiling-fetch) | ||
|
|
||
| You can set a cluster-wide default `timeout` for all search requests. This is configured by the `search.default_search_timeout` cluster setting. This setting defaults to `-1` indicating disabled or no timeout. This cluster-wide time-out is used as fallback if no `timeout` argument is designated by a search request. You can override this to a desired [time unit](elasticsearch://reference/elasticsearch/rest-apis/api-conventions.md#time-units) value using the [update cluster settings API](https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-cluster-put-settings). In this case, the request will be cancelled using the [task cancellation API](https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-tasks-cancel). For example | ||
|
|
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.
This is still quite hairy ;)
What about something like:
The timeout value applies per shard, starting when the query phase begins on that shard. It does not enforce an overall search-level timeout. If a shard exceeds the timeout, it returns partial results and the search response is marked timed_out: true.
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 feedback! This is delineated because it's what Support spends time clarifying from the doc not currently saying (specifically the whole bullet list of what doesn't qualify, which is actually what started this whole PR flow) 🙂.
FWIW maybe that's part of what @leemthompo's comment about language clean up 🙃
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.
It's too detailed and not up to date to my taste. The only thing that we should clarify is that it's applied per shard and we check it on a best effort basis every thread_pool.estimated_time_interval.
| ### Example [search-timeout-example] | ||
|
|
||
| To demonstrate the impact of the `timeout` parameter, consider an [async search](async-search-api.md). Async searches are designed for long-running searches, but you can use the `timeout` parameter to specify a duration you’d like to wait on each shard to complete. The overall latency of a search request depends on the number of shards needed for the search and the number of concurrent shard requests. Each shard collects hits within the specified time period. If collection isn’t finished when the period ends, {{es}} uses only the hits accumulated up to that point. | ||
|
|
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 am not sure that async search is the best example for this. This is generally used in scenario where latency is crucial.
Can we use a simple example with few shards, showing that some shards may return incomplete results?
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 am not sure that async search is the best example for this.
It is the existing example (although it only had a request body no response body); 🤷♀️ I have no horse in the race but also it doesn't really affect the response body example IMO.
Can we use a simple example with few shards, showing that some shards may return incomplete results?
I may need your help with a "more valid" example. This was simplified patterned from this real life which AFAICT looks par (that shards all "successful" but still timed_out: true). 🙏
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 am just proposing to change _async_search to _search, the example is fine.
👋🏽 howdy, team!
This updates the The
_searchAPI > Search Timeout documentation section which causes user confusion and induces Support volume. TLDR it looks like we had aspirations to clarify this recently in elastic/elasticsearch#47716 but need to go a bit farther.Statements pulled from
This does not include @\javanna's excellent callout here that some searches also cancel on connection closed.
🙋♀️ This PR should be confirmed by a Dev before merging.
TIA! Stef