-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Introduce async search status API #62947
Introduce async search status API #62947
Conversation
Pinging @elastic/es-search (:Search/Search) |
fb4373c
to
5a9b42b
Compare
Introduce async search status API GET /_async_search/status/<id> The API is restricted to the monitoring_user role. For a running async search, the response is: ```js { "id" : <id>, "is_running" : true, "start_time_in_millis" : 1583945890986, "expiration_time_in_millis" : 1584377890986, "_shards" : { "total" : 562, "successful" : 188, "skipped" : 0, "failed" : 0 } } ``` For a completed async search, the response is: ```js { "id" : <id> "is_running" : false, "expiration_time_in_millis" : 1584377890986 } ``` ---- Techincal details: We first try to retrieve the status of the async search from tasks. If this doesn't succeed, we retrieve it from an index: .async-search. In case of retrieving from the index, we assume that the async search is completed, and a shorter response for the status is returned. Closes elastic#57537
5a9b42b
to
faafd18
Compare
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.
Sorry for the late review @mayya-sharipova , I did a first pass and left some comments.
* @param expirationTime – expiration time of async search request | ||
* @return response representing the status of async search | ||
*/ | ||
AsyncStatusResponse toStatusResponse(String asyncExecutionId, long startTime, long expirationTime) { |
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 method should be synchronized.
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 Jim, good comment. I have made some variables volatile
and I believe it doesn't need synchronization any more.
.../plugin/async-search/src/main/java/org/elasticsearch/xpack/search/MutableSearchResponse.java
Outdated
Show resolved
Hide resolved
* is registered in the task manager, <code>null</code> otherwise. | ||
* | ||
*/ | ||
<T extends AsyncTask> T getTaskStatus(TaskManager taskManager, AsyncExecutionId asyncExecutionId, Class<T> tClass) { |
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.
Can you add an option on getTask
instead to enable/disable the authentication check ?
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 comment, @jimczi . I have talked to @albertzaharovits about this sometime ago, and he suggested me from a security point of view to use a separate method instead of adding a parameter to enable/disable authentication.
I ended up moving the logic of this method to TransportGetAsyncStatusAction
in order to avoid parametrized function arguments.
* @param listener – listener to report result to | ||
*/ | ||
public void getStatusResponse( | ||
AsyncExecutionId asyncExecutionId, BiFunction<String, Long, R> completedStatusProducer, ActionListener<R> listener) { |
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.
Can we reuse getEncodedResponse
? We'd need to disable the authentication check but that can be an option on the function directly ?
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.
Similarly to a previous comment, I opted out to have a separate method for this . Please let me know if we still want to have a single method.
...lugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncStatusResponse.java
Outdated
Show resolved
Hide resolved
@jimczi Jim, thanks for your review, I will go through it, and address it. I have another question. It seems that @lizozom and her team are interested in the BULK API for status. I remember we've discussed it, and we came to the conclusion that since we don't have a bulk API for @jimczi WDYT? Should we reconsider and add a bulk API for The API can look like this: GET /_async_search/status/
{
"ids" : [<id1>, <id2>]
} If we decide to go for a bulk version, we can remove a single version where id is provided in the URL. |
I think that we should first agreed on the metadata that we output for a single status. Once the format is settled, we can discuss whether a bulk format is needed or not but for now I think it's ok to focus on the API to get the status for a single 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.
The change looks good but I left some comments that need to be addressed.
.../plugin/async-search/src/main/java/org/elasticsearch/xpack/search/MutableSearchResponse.java
Outdated
Show resolved
Hide resolved
private final int successfulShards; | ||
private final int skippedShards; | ||
private final int failedShards; | ||
private final RestStatus completionStatus; |
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 think we can return the status for partial response too ?
...lugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncStatusResponse.java
Show resolved
Hide resolved
...async-search/src/main/java/org/elasticsearch/xpack/search/TransportGetAsyncStatusAction.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.
LGTM, thanks @mayya-sharipova!
if (queryFailures == null) { | ||
return 0; | ||
} | ||
int count = 0; |
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.
nit: you can do return queryFailures.asList().size()
?
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.
@lizozom I am wondering if you are ok with the format of the revised version of status response? |
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.
LGTM!
Thanks for the revision
Introduce async search status API GET /_async_search/status/<id> The API is restricted to the monitoring_user role. For a running async search, the response is: ```js { "id" : <id>, "is_running" : true, "is_partial" : true, "start_time_in_millis" : 1583945890986, "expiration_time_in_millis" : 1584377890986, "_shards" : { "total" : 562, "successful" : 188, "skipped" : 0, "failed" : 0 } } ``` For a completed async search, an additional `completion_status` fields is added. ```js { "id" : <id>, "is_running" : false, "is_partial" : false, "start_time_in_millis" : 1583945890986, "expiration_time_in_millis" : 1584377890986, "_shards" : { "total" : 562, "successful" : 562, "skipped" : 0, "failed" : 0 }, "completion_status" : 200 } ``` Closes elastic#57537 Backport for elastic#62947
This was missed in elastic#62947 Relates to elastic#62947
Introduce async search status API GET /_async_search/status/<id> The API is restricted to the monitoring_user role. For a running async search, the response is: ```js { "id" : <id>, "is_running" : true, "is_partial" : true, "start_time_in_millis" : 1583945890986, "expiration_time_in_millis" : 1584377890986, "_shards" : { "total" : 562, "successful" : 188, "skipped" : 0, "failed" : 0 } } ``` For a completed async search, an additional `completion_status` fields is added. ```js { "id" : <id>, "is_running" : false, "is_partial" : false, "start_time_in_millis" : 1583945890986, "expiration_time_in_millis" : 1584377890986, "_shards" : { "total" : 562, "successful" : 562, "skipped" : 0, "failed" : 0 }, "completion_status" : 200 } ``` Closes #57537 Backport for #62947
Introduce async search status API
GET /_async_search/status/
The API is restricted to the monitoring_user role.
Revised version:
For a running async search, the response is:
For a completed async search, an additional
completion_status
fields is added.Old version:
For a running async search, the response is:
For a completed async search, the response is:
Techincal details:
We first try to retrieve the status of the async search from tasks.
If this doesn't succeed, we retrieve it from an index: .async-search.
In case of retrieving from the index, we assume that the async search is
completed, and a shorter response for the status is returned.
Closes #57537