Refactor TransportSearchAction to allow run can_match exclusively #95064
Conversation
dnhatn
added
>non-issue
:Search/Search
dnhatn
changed the title
|
Pinging @elastic/es-search (Team:Search) |
| ); | ||
| }; | ||
| return searchAsyncAction; | ||
| AbstractSearchAsyncAction<? extends SearchPhaseResult> searchAsyncAction = switch (searchRequest.searchType()) { |
There was a problem hiding this comment.
you can return what the switch returns directly.
There was a problem hiding this comment.
++, I fixed it in 18b7182.
|
|
||
| runCoordinatorRewritePhase(); | ||
| if (getNumShards() == 0) { | ||
| finishPhase(); |
There was a problem hiding this comment.
I believe you added this if because you removed the same check that happens before at the beginning of the start method. As far as I can see, runCoordinatorRewritePhase loops over the iterators which does nothing if they are empty, and then calls finishPhase. That makes me wonder if we need this.
There was a problem hiding this comment.
Yes, I removed it in e412458.
| ) | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
+1 on removing this if not needed. The only reason I can think of that this was here is to return as early as possible and avoid calling executor.execute rather than returning a little later.
If I had to pick between this check and the one you added above (provided we need one of them, which we maybe don't?), I would keep this one as it happens earlier. Maybe we could call finishPhase directly.
There was a problem hiding this comment.
Good suggestion. I added the early finish back in 18b7182.
|
run elasticsearch-ci/part-3 |
|
@javanna Thanks for the review. |
I am currently testing the privileges of
cluster_search_shardsand evaluating whether we should create a new transport action or enhance the current one. In either scenario, we will require the capability to exclusively run the can_match phase using TransportSearchAction. I opened this pull request containing only the required changes (spun-off from #94534). This will enable us to review the change while we decide on the transport action option.Relates #94534