Modify SearchShards API and TransportSearchAction for CPS when MRT=false

[pawankartik-elastic]

This PR enables CPS for both _search and _async_search when ?ccs_minimize_roundtrips=false.

TODO:

• Reconcile the Clusters count after shard responses are pieced back.
• Ensure CI passes with prelim changes,
• Link with Serverless tests,
• Review.

Notes to myself:

• Perhaps, could swap out the transport version for a newer dedicated one?
  • Yes, I've swapped out the transport version to use a dedicated newer one.

[pawankartik-elastic]

2 other ways to potentially do the same thing:

1. Make SearchResponse.Cluster's members mutable, i.e. non-final and keep modifying them on the fly, or,
2. Do the reconciliation at the end. This would mean sprinkling CPS-specific code in the progress listeners or *Task.

Making members mutable to accommodate something doesn't feel right to me, and IIRC, we had a brief discussion earlier this year where a point was made that the progress listener is already complex enough due to the more and more code leaking into it (this was back when I was fixing the NPE bug in the async search). This approach seems simpler, IMO.

[elasticsearchmachine]

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

[quux00]

Partial first pass review. (Didn't have time to review everything yet)
Submitting my initial comments for now.
Overall this is looking good!

[quux00]

Do you need to check whether there are any origin project indices to search before returning EMPTY here? Or does shardResponses also include the origin project?

[pawankartik-elastic]

No, shardResponses only contains responses from linked projects. I'm not checking for the origin project here because we don't create a Clusters object if the search effectively becomes a local search (irrespective of whether we find local shards later on), since we don't have to display the metadata in the response. Here's the same behaviour for stateful currently on main:

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

Line 491 in fb70dd8

SearchResponse.Clusters.EMPTY,

[quux00]

OK, makes sense. Let's add a code comment stating that, since in this separate method the context is not immediately clear.

[quux00]

Why do we need a concurrent map here? Can this be called by more than one thread within a given request?

[pawankartik-elastic]

CHM wasn't used until: #99101. We only started using it after: #100129. I'm simply retaining the existing behaviour that's introduced as an enhancement.

[quux00]

The map passed into return new SearchResponse.Clusters(reconciledMap, false); (at the bottom of this method) is not used directly - the entries are copied into a ConcurrentHashMap in the SearchResponse.Clusters constructor. So unless this reconcileProjects method is ever called by multiple threads for a given request (which I don't think it is), then there is no need for this map to be concurrent. You are paying additional overhead for no reason.

[pawankartik-elastic]

This is SearchResponse.Clusters:

public Clusters(Map<String, Cluster> clusterInfoMap, boolean ccsMinimizeRoundtrips) {
    assert clusterInfoMap.size() > 0 : "this constructor should not be called with an empty Cluster info map";
    this.total = clusterInfoMap.size();
    this.clusterInfo = clusterInfoMap;
    this.successful = getClusterStateCount(Cluster.Status.SUCCESSFUL);
    this.skipped = getClusterStateCount(Cluster.Status.SKIPPED);
    // should only be called if "details" section of fromXContent is present (for ccsMinimizeRoundtrips)
    this.ccsMinimizeRoundtrips = ccsMinimizeRoundtrips;
}

In this ctor, the passed map is directly assigned to the field. I don't think this field is initialised prior and a copying is happening here. Or am I misreading this?

[quux00]

Oh, OK. This is worse than I thought :-) and points out a fragility in the SearchResponse.Clusters code, so thanks for pointing that out.

I was referring to this constructor (and the one that takes in a StreamInput).

The constructors that take in Map<String, Cluster> are fragile - you need to pass in ConcurrentHashMap and the arg signature of the function should require ConcurrentHashMap (not Map) OR those need to be refactored to copy from the incoming map into a ConcurrentHashMap. Otherwise we risk buggy code.

I think the proper fix is two-fold:

1. The Map type in the SearchResponse.Clusters class should be declared as ConcurrentHashMap, not Map, otherwise the code is fragile. I think that could be a follow-on refactoring PR that we leave for later.

2. The current constructor (in main) that takes Map<String, Cluster> (see below), should be changed to copy out of the incoming map into a new ConcurrentHashMap.

        public Clusters(Map<String, Cluster> clusterInfoMap) {
            assert clusterInfoMap.size() > 0 : "this constructor should not be called with an empty Cluster info map";
            this.total = clusterInfoMap.size();
            this.clusterInfo = clusterInfoMap;
            this.successful = getClusterStateCount(Cluster.Status.SUCCESSFUL);
            this.skipped = getClusterStateCount(Cluster.Status.SKIPPED);
            // should only be called if "details" section of fromXContent is present (for ccsMinimizeRoundtrips)
            this.ccsMinimizeRoundtrips = true;
        }

For your PR, that constructor was changed to call your new one, so let's change your new one to do the copying. In which case the code above on line 1023 should just be a Map not a ConcurrentHashMap.

[quux00]

Next round of feedback on the ConcurrentHashMap discussion.

[quux00]

Oh, OK. This is worse than I thought :-) and points out a fragility in the SearchResponse.Clusters code, so thanks for pointing that out.

I was referring to this constructor (and the one that takes in a StreamInput).

The constructors that take in Map<String, Cluster> are fragile - you need to pass in ConcurrentHashMap and the arg signature of the function should require ConcurrentHashMap (not Map) OR those need to be refactored to copy from the incoming map into a ConcurrentHashMap. Otherwise we risk buggy code.

I think the proper fix is two-fold:

1. The Map type in the SearchResponse.Clusters class should be declared as ConcurrentHashMap, not Map, otherwise the code is fragile. I think that could be a follow-on refactoring PR that we leave for later.

2. The current constructor (in main) that takes Map<String, Cluster> (see below), should be changed to copy out of the incoming map into a new ConcurrentHashMap.

        public Clusters(Map<String, Cluster> clusterInfoMap) {
            assert clusterInfoMap.size() > 0 : "this constructor should not be called with an empty Cluster info map";
            this.total = clusterInfoMap.size();
            this.clusterInfo = clusterInfoMap;
            this.successful = getClusterStateCount(Cluster.Status.SUCCESSFUL);
            this.skipped = getClusterStateCount(Cluster.Status.SKIPPED);
            // should only be called if "details" section of fromXContent is present (for ccsMinimizeRoundtrips)
            this.ccsMinimizeRoundtrips = true;
        }

For your PR, that constructor was changed to call your new one, so let's change your new one to do the copying. In which case the code above on line 1023 should just be a Map not a ConcurrentHashMap.

[quux00]

LGTM!