`ccs_minimize_roundtrips` should default to `true` for async search for CPS #135614

pawankartik-elastic · 2025-09-29T11:00:28Z

MRT defaults to true for _search and false for _async_search. However, we'd want both to default to true for CPS.
An error is returned should the user try to set the value manually.

Fulfils CPS requirement S2D18.

…or CPS MRT defaults to `true` for _search and `false` for _async_search. However, we'd want it to default to `true` for both for CPS. Fulfills CPS requirement S2D18.

server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

elasticsearchmachine · 2025-09-30T17:54:39Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

quux00

First review completed with changes requested.

server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java

.../lang-mustache/src/main/java/org/elasticsearch/script/mustache/RestSearchTemplateAction.java

quux00 · 2025-09-30T19:09:10Z

modules/reindex/src/main/java/org/elasticsearch/reindex/AbstractBulkByQueryRestHandler.java

+                parser,
+                clusterSupportsFeature,
+                size -> failOnSizeSpecified(),
+                Optional.empty()


Do we know that none of the subclasses of this class are cross-project enabled? We'll need to track those down and ensure that this Optional.empty() is valid for all. Otherwise, that arg will need to be added to the parseInternalRequest method signature.

Irrespective of whether they're CPS-enabled or not, Optional.empty() could be a valid argument since:

Their behaviour does not change for this value, and,

This value can be a placeholder to denote that an endpoint does not need any CPS-specific handling during parsing.

Passing in true or false would've marked their behaviour more explicitly. I'd prefer evaluating their CPS compatibility and updating this value in a separate PR. Wdyt?

I don't think I follow. If any CPS-enabled endpoints are using this AbstractBulkByQueryRestHandler class as a parent, then they must not pass in Optional.empty() because setting ccs_minimize_roundtrips is not allowed in that case. I doubt any bulk endpoints are CCS enabled, but we need to track them down to make sure.

The two endpoints using AbstractBulkByQueryRestHandler are _update_by_query and _delete_by_query. They accept CCS patterns such as foo*,remote*:bar*. However, the remotes included are not affected. For example, /*,*:*/_delete_by_query does not affect the remotes' docs. Instead, I see weird errors. If I specify a single remote, I no longer get any errors, but the docs still don't get deleted. I'm assuming this is an oversight, and that these endpoints don't play well with Cross Cluster ops in the first place. In that case, Optional.empty() is the right value here.

IMO, we should prevent Cross Cluster ops for them like we're planning to do for Fleet searches.
Edit: I just found tickets that track this internally.

server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java

x-pack/plugin/fleet/src/main/java/org/elasticsearch/xpack/fleet/rest/RestFleetSearchAction.java

.../plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/rest/RestRollupSearchAction.java

...n/async-search/src/main/java/org/elasticsearch/xpack/search/RestSubmitAsyncSearchAction.java

quux00 · 2025-09-30T20:30:12Z

Also there aren't many tests added (even including the 2 you did the serverless env). I think I'd like to see details of manual testing of all the REST endpoints changed here to ensure that the ccs_minimize_roundtrips defaults come out as expected and also whether you are allowed to set it.

pawankartik-elastic · 2025-10-01T16:32:33Z

Also there aren't many tests added (even including the 2 you did the serverless env). I think I'd like to see details of manual testing of all the REST endpoints changed here to ensure that the ccs_minimize_roundtrips defaults come out as expected and also whether you are allowed to set it.

I've refreshed the test in the serverless env, and here are the manual testing details: when CPS=true, _async_search, _search, and _search/template return an error only when MRT is explicitly set to either true or false. For CPS=false, there's no error whatsoever, irrespective of whether MRT is explicitly set or not. There's no change in the fleet endpoint's behaviour since CPS does not apply to it. I shared this matrix with you privately.

However, I have a question: does _rollup_search accept MRT value? It's not listed in its docs, but I don't get an error when I pass it. If I set MRT to false, I can see this value in TransportRollUpSearchAction, but it gets flipped to true when it goes through TSA.

quux00 · 2025-10-01T18:59:27Z

However, I have a question: does _rollup_search accept MRT value? It's not listed in its docs, but I don't get an error when I pass it. If I set MRT to false, I can see this value in TransportRollUpSearchAction, but it gets flipped to true when it goes through TSA.

Since it already accepts it (even if inappropriately) and rollup-search is not CSP enabled (and IIRC is not exposed in serverless?), we can just leave it as is.

piergm

Clean changes @pawankartik-elastic LGTM!

ccs_minimize_roundtrips should default to true for async search f…

972f34e

…or CPS MRT defaults to `true` for _search and `false` for _async_search. However, we'd want it to default to `true` for both for CPS. Fulfills CPS requirement S2D18.

pawankartik-elastic added >refactoring Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/CCS v9.2.0 labels Sep 29, 2025

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Sep 29, 2025

pawankartik-elastic added 2 commits September 29, 2025 21:25

Merge branch 'main' into pkar/cps-mrt-default-true

a72a5d9

Merge branch 'main' into pkar/cps-mrt-default-true

10662e6

pawankartik-elastic commented Sep 29, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java Show resolved Hide resolved

pawankartik-elastic commented Sep 29, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java Outdated Show resolved Hide resolved

pawankartik-elastic added 2 commits September 30, 2025 18:15

Set MRT when parsing instead of waiting for TSA

80461af

Merge branch 'main' into pkar/cps-mrt-default-true

2b3117d

pawankartik-elastic marked this pull request as ready for review September 30, 2025 17:54

pawankartik-elastic requested review from a team as code owners September 30, 2025 17:54

pawankartik-elastic requested a review from quux00 September 30, 2025 17:54

Merge branch 'main' into pkar/cps-mrt-default-true

6ae750a

quux00 reviewed Sep 30, 2025

View reviewed changes

pawankartik-elastic added 4 commits October 1, 2025 12:48

Address some review comments

f35bf00

Add comments to endpoints clarifying CPS context argument

264302e

Propagate CPS context value

fcf4f09

Add clarifying comments for CPS context argument

fbffdfe

elasticsearchmachine added v9.3.0 and removed v9.2.0 labels Oct 2, 2025

pawankartik-elastic requested a review from piergm October 2, 2025 14:10

piergm approved these changes Oct 3, 2025

View reviewed changes

Merge branch 'main' into pkar/cps-mrt-default-true

d2306a4

pawankartik-elastic merged commit 391e78d into elastic:main Oct 3, 2025
35 checks passed

This was referenced Oct 8, 2025

Revert "ccs_minimize_roundtrips should default to true for async search for CPS" for now #136198

Merged

Integrate with the new resolve_cross_project IndicesOptions #136463

Merged

ccs_minimize_roundtrips should default to true for async search for CPS #135614

ccs_minimize_roundtrips should default to true for async search for CPS #135614

Uh oh!

Conversation

pawankartik-elastic commented Sep 29, 2025

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 30, 2025

Uh oh!

quux00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quux00 Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

pawankartik-elastic Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

quux00 Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

pawankartik-elastic Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

pawankartik-elastic Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quux00 commented Sep 30, 2025

Uh oh!

pawankartik-elastic commented Oct 1, 2025

Uh oh!

quux00 commented Oct 1, 2025

Uh oh!

piergm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`ccs_minimize_roundtrips` should default to `true` for async search for CPS #135614

`ccs_minimize_roundtrips` should default to `true` for async search for CPS #135614

pawankartik-elastic Oct 2, 2025 •

edited

Loading