Skip to content

Conversation

@Mikep86
Copy link
Contributor

@Mikep86 Mikep86 commented Nov 14, 2025

Adds methods to QueryRewriteContext for registering and executing remote cluster async actions as part of the query rewrite process.

This functionality will be used by the semantic and intercepted (match/sparse_vector/knn) queries to get remote cluster inference results when performing CCS with ccs_minimize_roundtrips=false. It will also be used by the simplified linear/rrf retrievers to handle CCS. See the semantic search CCS POC for more context.

Both of these use cases will be implemented in follow-up PRs.

@Mikep86 Mikep86 requested review from a team, javanna and jimczi November 14, 2025 21:55
@Mikep86 Mikep86 added >non-issue :SearchOrg/Relevance Label for the Search (solution/org) Relevance team labels Nov 14, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/search-relevance (Team:Search - Relevance)

@elasticsearchmachine elasticsearchmachine added v9.3.0 Team:Search - Relevance The Search organization Search Relevance team labels Nov 14, 2025
@Mikep86
Copy link
Contributor Author

Mikep86 commented Nov 14, 2025

@javanna

@jimczi asked me to add you to this PR to confirm that this will work with security

Copy link
Member

@kderusso kderusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look reasonable to me, are there tests we can add here?

@Mikep86
Copy link
Contributor Author

Mikep86 commented Nov 17, 2025

@kderusso Tests were added in QueryRewriteContextMultiClustersIT, is there anything missing there?

@kderusso
Copy link
Member

@Mikep86 Sorry for the confusion, were there any additional unit tests that could make sense.

@Mikep86
Copy link
Contributor Author

Mikep86 commented Nov 17, 2025

@kderusso Unit tests for QueryRewriteContext are tough because there's a ton of boilerplate you need to implement to create an instance for testing. And that's even before we consider all the mocking we would need to do to test async actions in a unit test. Due to all of this, I went straight to integration tests where we can use a real QueryRewriteContext instance and unmocked async actions.

@kderusso
Copy link
Member

That's fair, I wanted to make sure we're thinking of it. Will defer to other reviewers for other concerns.

@Mikep86
Copy link
Contributor Author

Mikep86 commented Nov 19, 2025

Moving this back into draft as I work on modifying the implementation and test to handle security

@Mikep86 Mikep86 marked this pull request as draft November 19, 2025 21:28
@Mikep86
Copy link
Contributor Author

Mikep86 commented Nov 19, 2025

@elasticmachine update branch

@Mikep86
Copy link
Contributor Author

Mikep86 commented Nov 20, 2025

@elasticmachine update branch

@Mikep86
Copy link
Contributor Author

Mikep86 commented Nov 20, 2025

Hey all, this is ready for review again. I made changes to AbstractMultiClustersTestCase and TestCluster to allow them to support security:

  • Use an internal client to perform internal cluster management operations. When security is enabled, this internal client needs to use an origin with sufficient privileges, which is enabled by calling internalClientOrigin.
  • Update AbstractMultiClustersTestCase to allow NodeConfigurationSource to be configured. SecuritySettingsSource is a convenient collection of all the settings required to enable security, and this change allows us to integrate it cleanly.

@Mikep86 Mikep86 marked this pull request as ready for review November 20, 2025 19:42
Comment on lines +249 to +261
private void deleteSecurityIndex(String clusterAlias) {
final Client client = new OriginSettingClient(client(clusterAlias), SECURITY_ORIGIN);

GetIndexRequest getIndexRequest = new GetIndexRequest(TEST_REQUEST_TIMEOUT);
getIndexRequest.indices(SECURITY_MAIN_ALIAS);
getIndexRequest.indicesOptions(IndicesOptions.lenientExpandOpen());
GetIndexResponse getIndexResponse = client.admin().indices().getIndex(getIndexRequest).actionGet(TEST_REQUEST_TIMEOUT);

if (getIndexResponse.getIndices().length > 0) {
DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(getIndexResponse.getIndices());
assertAcked(client.admin().indices().delete(deleteIndexRequest).actionGet(TEST_REQUEST_TIMEOUT));
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@kderusso kderusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, as long as we have sufficient owner reviewers as well

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Mikep86 Mikep86 merged commit 7e06534 into elastic:main Nov 24, 2025
34 checks passed
afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Nov 26, 2025
ncordon pushed a commit to ncordon/elasticsearch that referenced this pull request Nov 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :SearchOrg/Relevance Label for the Search (solution/org) Relevance team Team:Search - Relevance The Search organization Search Relevance team v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants