-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Introduce eql search status API #68065
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 eql search status API #68065
Conversation
Introduce eql search status API, that reports the status of eql stored or async search. GET _eql/search/status/<id> The API is restricted to the monitoring_user role. For a running eql search, a response has the following format: { "id" : <id>, "is_running" : true, "is_partial" : true, "start_time_in_millis" : 1611690235000, "expiration_time_in_millis" : 1611690295000 } For a completed eql search, a response has the following format: { "id" : <id>, "is_running" : false, "is_partial" : false, "expiration_time_in_millis" : 1611690295000, "completion_status" : 200 } Closes elastic#66955
Pinging @elastic/es-search (Team:Search) |
Pinging @elastic/es-ql (Team:QL) |
@lizozom Unfortunately for eql searches, we could not the same response format as for async searches, as we don't store shards progress for eql searches. I hope the format presented above works for you. |
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.
Looks good to me in general. Left a couple of comments. I think it would be great if @costin could also take a look.
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlStatusResponse.java
Show resolved
Hide resolved
/** | ||
* An interface for status response of the stored or running async search | ||
*/ | ||
public interface SearchStatusResponse { |
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.
There is quite a bit of code repetition between AsyncStatusResponse and EqlStatusResponse, and between TransportEqlAsyncGetStatusAction and TransportGetAsyncStatusAction, any reason for not moving more shared things into SearchStatusResponse and AsyncTaskIndexService respectively?
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.
@imotov Thank for the feedback, addressed in 4321efb. Can you please continue with the review when you have time.
I was not able to combine AsyncStatusResponse and EqlStatusResponse together because it would complicate the wire serialization/deserialization code. As we already have AsyncStatusResponse
form 7.11, I did not want to change that part of the code.
...n/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlAsyncGetStatusAction.java
Outdated
Show resolved
Hide resolved
@imotov @astefan Thanks for reviewing the PR. I will study and address your comments. |
LGTM to me from my end though I'm not familiar with the async code so I'm non-binding :) Line 250 in ef26b53
However it should be trivial to signal a new match during the process. |
@costin Thank you for checking.
For a partial information for a status report, we don't need partial results, Kibana just needs progress indication about the number of the completed shards so far and total shards. |
@elasticmachine update branch |
- Add serializing tests for EqlStatusResponse and AsyncStatusResponse - Move shared code of TransportEqlAsyncGetStatusAction and and TransportGetAsyncStatusAction to AsyncTaskIndexService - Use clusterService.state().getNodes().getLocalNode() to get a local node
Also remove skip section in yml tests, as these tests are not used for BWC testings
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 modifications! Left a small comment about a potential race condition in the integration test. Otherwise LGTM.
- match: { is_running: true } | ||
- match: { is_partial: 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.
These two checks will probably fail occasionally due to a race condition with task completion.
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.
Introduce eql search status API, that reports the status of eql stored or async search. GET _eql/search/status/<id> The API is restricted to the monitoring_user role. For a running eql search, a response has the following format: { "id" : <id>, "is_running" : true, "is_partial" : true, "start_time_in_millis" : 1611690235000, "expiration_time_in_millis" : 1611690295000 } For a completed eql search, a response has the following format: { "id" : <id>, "is_running" : false, "is_partial" : false, "expiration_time_in_millis" : 1611690295000, "completion_status" : 200 } Backport for #68065 Closes #66955
A commit from #68065 missed a parameter description
A commit from elastic#68065 missed a parameter description
Introduce eql search status API,
that reports the status of eql stored or async search.
GET _eql/search/status/
The API is restricted to the monitoring_user role.
For a running eql search, a response has the following format:
For a completed eql search, a response has the following format:
Closes #66955