Add support for merging multiple search responses into one #37566
Conversation
This will be used in cross-cluster search when reduction will be performed locally on each cluster. The CCS coordinating node will send one search request per remote cluster involved and will get one search response back from each one of them. Such responses contain all the info to be able to perform an additional reduction and return results back to the user. Relates to elastic#32125
javanna
added
>enhancement
WIP
:Search/Search
|
Pinging @elastic/es-search |
| * and pipeline aggregations have not yet been executed. Also, from+size search hits need to be requested to each cluster. | ||
| */ | ||
| //TODO it may make sense to investigate reusing existing merge code in SearchPhaseController#reducedQueryPhase, the logic is similar | ||
| //yet there are substantial differences in terms of the objects exchanged and logic in the sortDocs method. |
There was a problem hiding this comment.
It's more than just reusing the reducedQueryPhase, as we discussed earlier we could integrate the remote cluster response as a shard response in the initial search phase and ignore hits coming from the remote cluster in the fetch phase. This would be identical to the removed QueryAndFetch strategy except that only the remote cluster response would have the fetch results. This is really a nice to have so no need to follow up on this but it would be nice if the TODO mentions this.
| /** | ||
| * Add a search response to the list of responses to be merged together into one. | ||
| * Merges currently happen at once when all responses are available and {@link #getMergedResponse()} is called. That may change | ||
| * in the future as it's possible to introduce incremental merges as responses come in if necessary. |
There was a problem hiding this comment.
I don't think incremental merges is appealing here. The number of remote clusters should be low so there is no benefit to do the reduce incrementally.
There was a problem hiding this comment.
I think we may come back to this and have a laugh one day about the "number of remote clusters should be low" assumption :) but I agree we are not talking about hundreds at the moment. I thought it may be useful to incrementally reduce given the size of the responses from each cluster, but we should first measure what the benefit is if any.
|
|
||
| TotalHits totalHits = null; | ||
| if (totalHitsRelation != null) { | ||
| //TODO totalHits may overflow if each cluster reports a very high number? |
There was a problem hiding this comment.
@jimczi do you have thoughts on this one? is it paranoia on my end?
There was a problem hiding this comment.
totalHits.value is a long so I doubt that it will overflow unless you have millions of remote cluster ;). However this made me think that this pr doesn't handle track_total_hits when it is set as a number. In the normal execution we'll merge all the topdocs and if the resulting total hits is greater than track_total_hits we set the final value as the value in track_total_hits and the final relation to gte. You can check the logic here
We'll also need to change the code when #37466 is merged since the default for
track_total_hits is going to change.
There was a problem hiding this comment.
Good point, I addressed that in a325129 . I think that since I reuse TopDocsStats, this will not need to change once the default trackTotalHitsUpTo changes.
I also went a step further in 8f1b063 and expanded TopDocsStats. Just an experiment but it saves some duplicated logic, let me know what you think.
There was a problem hiding this comment.
thanks, the change looks good. Feel free to push when CI is green
Adapt TopDocsStats so it can be reused.
This allows to share more code between SearchResponseMerger and SearchPhaseController
… known once all responses have been obtained from the remote clusters
|
retest this please |
1 similar comment
|
retest this please |
|
I am going to merge this PR despite the failed builds. There are problems in the elasticsearch CI infra that are causing a lot of build failures. I have run this PR multiple times on my own CI and everything was green which gives me confidence that I can merge it. |
With #37566 we have introduced the ability to merge multiple search responses into one. That makes it possible to expose a new way of executing cross-cluster search requests, that makes CCS much faster whenever there is network latency between the CCS coordinating node and the remote clusters. The coordinating node can now send a single search request to each remote cluster, which gets reduced by each one of them. from + size results are requested to each cluster, and the reduce phase in each cluster is non final (meaning that buckets are not pruned and pipeline aggs are not executed). The CCS coordinating node performs an additional, final reduction, which produces one search response out of the multiple responses received from the different clusters. This new execution path will be activated by default for any CCS request unless a scroll is provided or inner hits are requested as part of field collapsing. The search API accepts now a new parameter called ccs_minimize_roundtrips that allows to opt-out of the default behaviour. Relates to #32125
This will be used in cross-cluster search when reduction will be
performed locally on each cluster. The CCS coordinating node will send
one search request per remote cluster involved and will get one search
response back from each one of them. Such responses contain all the info
to be able to perform an additional reduction and return results back
to the user.
Relates to #32125