- Notifications
You must be signed in to change notification settings - Fork 25.6k
Add new search shards endpoint #94534
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
Conversation
| Hi @dnhatn, I've created a changelog YAML for you. |
| Hi @dnhatn, I've created a changelog YAML for you. |
| Pinging @elastic/es-search (Team:Search) |
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchShardsAction.java 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.
I left a couple of comments, while we decide what to do about privileges.
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchShardsAction.java Outdated Show resolved Hide resolved
| While the |
| @javanna I've updated this PR to call the can_match phase directly. However, I intentionally left out support for PIT to keep this PR small. I plan to add it in a follow-up. Can you please take a look? Thank you! |
| @ywangd Thank you so much for your help regarding the privilege. Our discussion was great, and based on your suggestions, I have made these two changes:
As a result of these changes, this API now requires the same privilege as the pre-existing cluster_search_shards API. Would you mind reviewing these changes? Thank 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.
LGTM
Nhat, thanks for the discussion. I appreciate you spending time reaching out to us on this topic. The action and privileges look right to me. 👍
While integrating the new search_shards API with CCS, I found that we had forgotten to add it to the allow list in the CrossClusterAccessServerTransportFilter so that we can call it in CCS. Also, I added the system privilege to this API so it can be called with the system user in field-caps and other APIs. Since we have many tests that will cover this API once it is integrated, I don't add any security-related tests specifically for this API in this PR. Relates #94534
We should use the search_coordinator threadpool when performing the can_match phase in the new search_shards API. Relates #94534
While integrating the new search_shards API with CCS, I encountered an issue where the search_shards operation returned a wrong result when using the _index:cluster:index_name filter. The reason for this failure is that we are not sending the cluster alias to the search_shards API, resulting in the search context lacking a cluster alias to match the index pattern filter. Relates #94534
The preFiltered flag aimed to facilitate a smooth transition from the old response to the new response. However, instead of serializing this flag over the wire, we prefer an alternative approach. Since the new API has not been used thus far, there is no need to handle backward compatibility or introduce a new transport version. Relates #94534
If the query cannot be rewritten to match_none, then search_shards API should skip the can_match phase and return all groups of shard copies. Relates #94534
When security is enabled, CCS requests for a remote wildcard pointing data streams are not resolved correctly. This issue happens because search_shards requests do not override the includeDataStreams method, causing the RBAC engine to exclude data streams while resolving the index pattern. Relates #95894 Relates #94534
This pull request introduces a new search shards endpoint that returns a list of shards where a search request will be executed. Unlike the existing cluster search_shards API, this API performs the can_match phase on both the coordinator and data nodes, which excludes non-matching shards from the results.
Once this pull request is merged, I will work on a follow-up to switch over to using this API in CCS search.
Relates #93730