Add minimum compatibility version to SearchRequest by astefan · Pull Request #65896 · elastic/elasticsearch

astefan · 2020-12-04T16:12:50Z

Adds a minimum version request parameter to SearchRequest. The minimum version helps failing a request if any shards involved in the search do not meet the compatibility requirements (all shards need to have a version equal or later than the minimum version provided).

Relates to #63304.

to REST. The minimum version helps failing a request if any shards involved in the search do not meet the compatibility requirements (all shards need to have a version equal or later than the minimum version provided).

elasticmachine · 2020-12-04T16:12:53Z

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

jimczi

I left some comments but the logic looks good to me.

jimczi · 2020-12-07T16:45:13Z

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

                                       final SearchActionListener<SearchPhaseResult> listener) {
        ShardSearchRequest request = rewriteShardSearchRequest(super.buildShardSearchRequest(shardIt));
-        getSearchTransport().sendExecuteQuery(getConnection(shard.getClusterAlias(), shard.getNodeId()), request, getTask(), listener);
+        Transport.Connection conn = getConnection(shard.getClusterAlias(), shard.getNodeId());


Can you move this to AbstractSearchAsyncAction#getConnection ? There's nothing that prevents to check the version there.

jimczi · 2020-12-07T16:46:20Z

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

        ShardSearchRequest request = rewriteShardSearchRequest(super.buildShardSearchRequest(shardIt));
-        getSearchTransport().sendExecuteQuery(getConnection(shard.getClusterAlias(), shard.getNodeId()), request, getTask(), listener);
+        Transport.Connection conn = getConnection(shard.getClusterAlias(), shard.getNodeId());
+        if (minVersion != null && conn.getVersion().before(minVersion)) {


conn can be null too so you'll need to handle this case.

jimczi · 2020-12-07T16:47:35Z

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

            finalReduce = true;
        }
        ccsMinimizeRoundtrips = in.readBoolean();
+        if (in.getVersion().onOrAfter(Version.V_7_11_0)) {


You need to start with 8.0.0 and adapt the checks after the backport

jimczi · 2020-12-07T16:47:42Z

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

        }
        out.writeBoolean(ccsMinimizeRoundtrips);
-
+        if (out.getVersion().onOrAfter(Version.V_7_11_0)) {


jimczi · 2020-12-07T16:49:03Z

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

                    throw new SearchPhaseExecutionException(getName(), msg, null, ShardSearchFailure.EMPTY_ARRAY);
                }
            }
+            if (checkMinimumVersion(shardsIts) == false) {


You don't need to check the minimum version if it's equal to the minimum compatible version (default value).

jimczi · 2020-12-07T16:49:46Z

server/src/main/java/org/elasticsearch/ElasticsearchException.java

+                org.elasticsearch.action.search.VersionMismatchException.class,
+                org.elasticsearch.action.search.VersionMismatchException::new,
+                161,
+                Version.V_7_11_0);


For now it's just a 8.0.0 exception, you'll need to adapt the version after the backport

…o 63304_part1

jimczi · 2020-12-08T09:01:56Z

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

        return new SearchRequest(originalSearchRequest, indices, clusterAlias, absoluteStartMillis, finalReduce);
    }

+    public static SearchRequest withMinimumVersion(SearchRequest searchRequest, Version minVersion) {


You could reuse the subSearchRequest above and just add a nullable minCompatibleShardNode argument ? That makes sense to me because we need to set ccsMinimizeRoundtrips to false there too and these sub search requests are meant for internal use (using the transport client only).
Should we add also a check that the min version is greater or equals than the minimum compatibility version ?

We should also ensure that ccsMinimizeRoundtrips is set to false, otherwise the check will not be applied on the remote cluster. We can force the value here searchRequest.cssMinimizeRountrips(false) but we should also verify the final value in SearchRequest#validate.

jimczi · 2020-12-08T09:14:47Z

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

@@ -319,6 +336,9 @@ long getAbsoluteStartMillis() {
        return absoluteStartMillis;
    }



Maybe rename to something more explicit: minCompatibleShardNode ?

…o 63304_part1

jimczi

The change looks great. Thanks for adding the mixed cluster tests. I left one comment regarding the ccs_minimize_roundtrips that could be unset automatically.

jimczi · 2021-01-04T16:52:44Z

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

@@ -97,6 +97,9 @@ public class SearchRequest extends ActionRequest implements IndicesRequest.Repla

    private boolean ccsMinimizeRoundtrips = true;


Should we switch to a Boolean with a default value of null ? This way we can decide the default value depending on whether the minCompatibleShardNode is set or not ? It feels wrong that you have to explicitly disable this option when using minCompatibleShardNode. It should be done automatically unless the user sets the ccsMinimizeRoundtrips value explicitly.

@jimczi I've made the change, but slightly different from your suggestion as it wasn't easy to enforce a certain value for ccsMinimizeRoundtrips in all cases. Thus, I've removed the setter and kept as the sole option of setting minCompatibleShardNode for a search request - a constructor (which will also set ccsMinimizeRoundtrips).

…o 63304_part1

jimczi

LGTM.
Thanks @astefan!

…o 63304_part1

use 8.0.0 as the version to test against.

…o 63304_part1

* Adds a minimum version request parameter to SearchRequest. The minimum version helps failing a request if any shards involved in the search do not meet the compatibility requirements (all shards need to have a version equal or later than the minimum version provided). (cherry picked from commit e3386e1)

Adds a minimum version parameter to the SearchRequest, not exposed

1c41a9c

to REST. The minimum version helps failing a request if any shards involved in the search do not meet the compatibility requirements (all shards need to have a version equal or later than the minimum version provided).

astefan added >feature :Search/Search Search-related issues that do not fall into other categories labels Dec 4, 2020

elasticmachine added the Team:Search Meta label for search team label Dec 4, 2020

jimczi requested changes Dec 7, 2020

View reviewed changes

astefan added 3 commits December 8, 2020 01:02

Address reviews

2bcb4aa

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

df89dcc

…o 63304_part1

Adjustment after merge

5410f26

astefan requested a review from jimczi December 8, 2020 08:18

jimczi reviewed Dec 8, 2020

View reviewed changes

astefan added 3 commits January 4, 2021 15:29

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

137c089

…o 63304_part1

Add min_compatible_shard_node as request parameter

c262054

Small fixes

7c6ab18

jimczi requested changes Jan 4, 2021

View reviewed changes

astefan added 2 commits January 5, 2021 17:31

Address reviews

e677515

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

e85c0b5

…o 63304_part1

jimczi approved these changes Jan 7, 2021

View reviewed changes

Differentiate between pre 7.12.0 mixed version cluster tests and post.

9a9a919

astefan mentioned this pull request Jan 12, 2021

Add minimum compatibility version to SearchRequest #67373

Closed

astefan added 2 commits January 12, 2021 19:15

Disable bwc tests.

b1acef2

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

3ef1cd0

…o 63304_part1

astefan added v7.12.0 v8.0.0 labels Jan 12, 2021

astefan added 3 commits January 12, 2021 19:39

Re-enable bwc tests

d51a352

Just to please the bwc checks, since the code is not in 7.x yet,

54365b7

use 8.0.0 as the version to test against.

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

6eadbde

…o 63304_part1

astefan merged commit e3386e1 into elastic:master Jan 12, 2021

astefan added the backport pending label Jan 12, 2021

astefan mentioned this pull request Jan 13, 2021

Add minimum compatibility version to SearchRequest #67414

Merged

astefan removed the backport pending label Jan 13, 2021

astefan mentioned this pull request Jan 13, 2021

Enable bwc tests after elastic#67414 merge #67430

Merged

astefan deleted the 63304_part1 branch January 13, 2021 15:11

This was referenced Feb 5, 2021

QL: retry SQL and EQL requests in a mixed-node (rolling upgrade) cluster #68602

Merged

QL: "fields" api implementation in QL #68802

Merged

stevejgordon mentioned this pull request Feb 22, 2021

7.12.0 Meta Ticket elastic/elasticsearch-net#5337

Closed

34 tasks

bpintea mentioned this pull request Jun 1, 2021

Versioning aware scripting #71194

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		@@ -319,6 +336,9 @@ long getAbsoluteStartMillis() {
		return absoluteStartMillis;
		}

		@@ -97,6 +97,9 @@ public class SearchRequest extends ActionRequest implements IndicesRequest.Repla

		private boolean ccsMinimizeRoundtrips = true;

Conversation

astefan commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 4, 2020

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi Dec 8, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

astefan commented Dec 4, 2020 •

edited

Loading

jimczi Dec 8, 2020 •

edited

Loading