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

Add completion_time time field to async_search get and status response #97700

Merged
merged 5 commits into from
Jul 17, 2023

Conversation

quux00
Copy link
Contributor

@quux00 quux00 commented Jul 14, 2023

Both the async_search response and status response objects now include a "completion_time_in_millis"
entry in the XContent output if the search has completed successfully.

The completion_time is set as the start_time (already present) plus the 'took' time that is set in the
SearchResponse object and only if the isRunning status == false since took is set even for in-progress searches.

We use the 'took' field because it is based on relative time, not absolute wall clock time which can go
backwards due to NTP issues. See the comments in TransportSearchAction about the SearchTimeProvider
for details.

Closes #88640

@quux00 quux00 added >enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.10.0 labels Jul 14, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine
Copy link
Collaborator

Hi @quux00, I've created a changelog YAML for you.

@quux00 quux00 added the WIP label Jul 14, 2023
@quux00 quux00 changed the title Add completion_time time field to async_search get and status response WIP: Add completion_time time field to async_search get and status response Jul 14, 2023
@quux00 quux00 force-pushed the async-search/completion-time branch from 695b8a2 to 2720025 Compare July 14, 2023 18:55
@quux00 quux00 changed the title WIP: Add completion_time time field to async_search get and status response Add completion_time time field to async_search get and status response Jul 14, 2023
@quux00 quux00 removed the WIP label Jul 14, 2023
@elasticsearchmachine
Copy link
Collaborator

Hi @quux00, I've created a changelog YAML for you.

@quux00 quux00 requested a review from javanna July 14, 2023 20:12
quux00 added 3 commits July 14, 2023 16:12
The completion_time is set as the start_time (already present) plus the 'took'
time that is set in the SearchResponse object and only if the isRunning status == false
since took is set even for in-progress searches.

We use the 'took' field because it is based on relative time, not absolute wall clock time
which can go backwards due to NPT issues. See the comments in TransportSearchAction about
the SearchTimeProvider for details.

Closes elastic#88640
…ion_time_in_millis entry.

Fixed docs tests to handle the new completion_time_in_millis in the response.
This required incrementing the TransportVersion as unlike the
AsyncResponse, the status response does not have access to the
searchResponse, which has the took value that is used to compute
the completion time in the AsyncResponse.
@quux00 quux00 force-pushed the async-search/completion-time branch from be81322 to 1cbade0 Compare July 14, 2023 20:14
@quux00 quux00 requested a review from mayya-sharipova July 14, 2023 20:15
@elasticsearchmachine
Copy link
Collaborator

Hi @quux00, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Hi @quux00, I've updated the changelog YAML for you.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM please sync up with Kibana to make sure they are notified that this is being implemented, and let them verify as well as migrate their code to use this new field.

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @quux00, this also LGTM

@quux00 quux00 merged commit eaa8679 into elastic:main Jul 17, 2023
quux00 added a commit to elastic/elasticsearch-specification that referenced this pull request Aug 29, 2023
Updates the spec for the response objects for `POST _async_search`, `GET _async_search/:id`,
`GET _async_search/status/:id` and `GET _search`

Two high level changes have happened in 8.10:

1) `completion_time` and `completion_time_in_millis` (async_search only) elastic/elasticsearch#97700

2) Adding `details` metadata to the `_clusters` section of both async and synchronous search responses elastic/elasticsearch#97731
quux00 added a commit to elastic/elasticsearch-specification that referenced this pull request Aug 31, 2023
Updates the spec for the response objects for `POST _async_search`, `GET _async_search/:id`,
`GET _async_search/status/:id` and `GET _search`

Two high level changes have happened in 8.10:

1) `completion_time` and `completion_time_in_millis` (async_search only) elastic/elasticsearch#97700

2) Adding `details` metadata to the `_clusters` section of both async and synchronous search responses elastic/elasticsearch#97731
swallez pushed a commit to elastic/elasticsearch-specification that referenced this pull request Sep 1, 2023
Updates the spec for the response objects for `POST _async_search`, `GET _async_search/:id`,
`GET _async_search/status/:id` and `GET _search`

Two high level changes have happened in 8.10:

1) `completion_time` and `completion_time_in_millis` (async_search only) elastic/elasticsearch#97700

2) Adding `details` metadata to the `_clusters` section of both async and synchronous search responses elastic/elasticsearch#97731
swallez pushed a commit to elastic/elasticsearch-specification that referenced this pull request Sep 1, 2023
Updates the spec for the response objects for `POST _async_search`, `GET _async_search/:id`,
`GET _async_search/status/:id` and `GET _search`

Two high level changes have happened in 8.10:

1) `completion_time` and `completion_time_in_millis` (async_search only) elastic/elasticsearch#97700

2) Adding `details` metadata to the `_clusters` section of both async and synchronous search responses elastic/elasticsearch#97731
swallez added a commit to elastic/elasticsearch-specification that referenced this pull request Sep 5, 2023
Updates the spec for the response objects for `POST _async_search`, `GET _async_search/:id`,
`GET _async_search/status/:id` and `GET _search`

Two high level changes have happened in 8.10:

1) `completion_time` and `completion_time_in_millis` (async_search only) elastic/elasticsearch#97700

2) Adding `details` metadata to the `_clusters` section of both async and synchronous search responses elastic/elasticsearch#97731

Co-authored-by: Michael Peterson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add completion_time time field to async_search get or status response
4 participants