-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Adds the _cluster object to the AsyncStatusResponse XContent when Clusters has a non-zero count total #96535
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
Adds the _cluster object to the AsyncStatusResponse XContent when Clusters has a non-zero count total #96535
Conversation
|
Pinging @elastic/es-search (Team:Search) |
|
Manual testing examples: Local cluster only searches do not show the whereas searches with one or more remote clusters (CCS) shows the _cluster object from both endpoints: The same is true for |
javanna
left a comment
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.
left a couple of questions
.../plugin/async-search/src/main/java/org/elasticsearch/xpack/search/MutableSearchResponse.java
Outdated
Show resolved
Hide resolved
...lugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncStatusResponse.java
Outdated
Show resolved
Hide resolved
4a75bd5 to
3fe8951
Compare
...lugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncStatusResponse.java
Outdated
Show resolved
Hide resolved
...lugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncStatusResponse.java
Outdated
Show resolved
Hide resolved
90dfa42 to
a0c1cee
Compare
javanna
left a comment
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.
left a question, LGTM otherwise
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.
just checking that you don't forget to remove this comment ;)
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.
Right, sorry. Removing it now. Thanks for the reminder :-)
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 needs to be nullable because older nodes won't send it over the wire. I am less clear on having it null for local-only searches. Shouldn't we use EMPTY in this case like we normally do?
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.
Currently (before this ticket) _clusters is not shown for local-only searches in the async-status/status response, so that has been the default already.
And in addition, I'm not sure why we use EMPTY for the normal non-CCS case, as that seems wrong to me.
This type of output would be confusing to end users:
{
"id": "FnhKR2FES241UmNtSjhGWUVTYkhGWWcaemJJaENTNW9SZFdaWnNmRUNzbE1VUTozNjU",
"is_running": false,
"is_partial": false,
"start_time_in_millis": 1686227754043,
"expiration_time_in_millis": 1686659754043,
"_shards": {
"total": 3,
"successful": 3,
"skipped": 0,
"failed": 0
},
"_clusters": {
"total": 0, // why is this zero?
"successful": 0, // didn't my local cluster search work?
"skipped": 0
},
"completion_status": 200
}
IMO, the standard non-CCS default Clusters object should be Clusters{total=1, successful=1, skipped=0}. Using empty implies that we don't want to count the local cluster as a cluster. But if that's true, then should we also not count it for CCS? But then that would be also confusing to end users.
If we add EMPTY here for the local-only, we could choose to not serialize it, but that requires extra logic in the code above or if we serialize it but don't want to show it, then that requires extra logic in the toXContent methods. So I chose the path of least resistance which is to just have it be null, which matches current functionality for local-only searches.
If you want EMPTY serialized, then I would say we need to add extra logic in toXContent to not output it to the REST response.
…sters has a non-zero count total. When CCS is done, the _cluster object will have a non-zero `total` count and thus is useful status information for the user. When a local cluster only async search is done, the _cluster object will not be present in the AsyncStatusResponse.
…bject so it can be obtained by get-async-search-status coordinators that are not the coordinator for the async search itself. This requires a Transport Version bump.
…nse when looking to set completion status. Added several new unit tests in AsyncStatusResponseTests
a0c1cee to
6ad8289
Compare
During a cross cluster search, the
_clusterobject in the SearchResponse will have a non-zerototalcount and thus is useful status information for the user. That information is included in the_async_search/<id>endpoint already because it is part of the underlying SearchResponse.However, that information is not present in the
_async_search/status/<id>endpoint, since that does not show the SearchResponse details. With a cross-cluster search this is useful information to understand (1) how many clusters are being searched; (2) whether they are successful or skipped, so it is being added in this commit.When an async search is performed against the local cluster only , the
_clusterobject will not be present in the AsyncStatusResponse, since it is not informative (TransportSearchAction uses Clusters.EMPTY for executeLocalSearch).