Make Persistent Tasks implementations version and feature aware #31045

bleskes · 2018-06-02T11:55:21Z

With #31020 we introduced the ability for transport clients to indicate what features they support in order to make sure we don't serialize object to them they don't support. This PR adapts the serialization logic of persistent tasks to be aware of those features and not serialize tasks that aren't supported.

Also, a version check is added for the future where we may add new tasks implementations and need to be able to indicate they shouldn't be serialized both to nodes and clients.

As the implementation relies on the interface of PersistentTaskParams, these are no longer optional. That's acceptable as all current implementation have them and we plan to make PersistentTaskParams more central in the future.

Relates to #30731

This commit introduces the ability for a client to communicate to the server features that it can support and for these features to be used in influencing the decisions that the server makes when communicating with the client. To this end we carry the features from the client to the underlying stream as we carry the version of the client today. This enables us to enhance the logic where we make protocol decisions on the basis of the version on the stream to also make protocol decisions on the basis of the features on the stream. With such functionality, the client can communicate to the server if it is a transport client, or if it has, for example, X-Pack installed. This enables us to support rolling upgrades from the OSS distribution to the default distribution without breaking client connectivity as we can now elect to serialize customs in the cluster state depending on whether or not the client reports to us using the feature capabilities that it can under these customs. This means that we would avoid sending a client pieces of the cluster state that it can not understand. However, we want to take care and always send the full cluster state during node-to-node communication as otherwise we would end up with different understanding of what is in the cluster state across nodes depending on which features they reported to have. This is why when deciding whether or not to write out a custom we always send the custom if the client is not a transport client and otherwise do not send the custom if the client is transport client that does not report to have the feature required by the custom. Co-authored-by: Yannick Welsch <yannick@welsch.lu>

…features

elasticmachine · 2018-06-02T11:55:23Z

Pinging @elastic/es-distributed

jasontedor

Overall it looks good. I left some suggestions and one required change.

jasontedor · 2018-06-02T12:00:08Z

server/src/main/java/org/elasticsearch/cluster/NamedDiffable.java

@@ -19,17 +19,10 @@

 package org.elasticsearch.cluster;

-import org.elasticsearch.Version;
-import org.elasticsearch.common.io.stream.NamedWriteable;
+import org.elasticsearch.common.io.stream.VersionedNamedWriteable;

 /**
 * Diff that also support NamedWriteable interface


This could should be modified to VersionedNamedWriteable now.

jasontedor · 2018-06-02T12:11:05Z

server/src/main/java/org/elasticsearch/cluster/RestoreInProgress.java

@@ -382,6 +383,11 @@ public String getWriteableName() {
        return TYPE;
    }

+    @Override
+    public Version getMinimalSupportedVersion() {
+        return Version.V_5_0_0;


It is unfortunate that this has to be littered everywhere and anyway it will become a lie when we remove the 5.x constants from the codebase on some future cleanup. Why not use Version#minimumCompatibilityVersion as a default implementation on the interface?

I felt that people should really think about these things and not just rely on defaults. Another aspect is that this change currently means it is littered everywhere in this PR but when people introduce a new VersionedNamedWritable the have to set something can't use a default. When 5.x is phased out we can replace it with Version#minimumCompatibilityVersion (I can do so now if you want) but I think it should be done on a case by case basis. Defaults are dangerous here imo.

Fair enough. Let's go with Version#minimumCompatibilityVersion everywhere now though.

I replaced all occurrences of V5_0_0. Let me know if you want the other 5.x replaced as well.

Yeah, let's get those here too.

server/src/test/java/org/elasticsearch/persistent/PersistentTasksCustomMetaDataTests.java

+            randomAssignment());
+        final BytesStreamOutput out = new BytesStreamOutput();
+        out.setVersion(streamVersion);
+        Set<String> features = new HashSet<>();


jasontedor · 2018-06-02T12:26:15Z

server/src/test/java/org/elasticsearch/persistent/PersistentTasksCustomMetaDataTests.java

+            new PersistentTasksCustomMetaData(new NamedWriteableAwareStreamInput(input, getNamedWriteableRegistry()));
+
+        Set<String> expectedIds = new HashSet<>(tasks.getCurrentTaskIds());
+        expectedIds.remove("test_incompatible_version");


This is indirect. Why not Collections.singleton("test_compatible_version")? Then the local is not even needed and it saves a few lines too.

Left over where I had more random tasks in there but I removed it due to other issues. Will simplify.

jasontedor · 2018-06-02T12:30:07Z

server/src/test/java/org/elasticsearch/persistent/PersistentTasksCustomMetaDataTests.java

+        PersistentTasksCustomMetaData read = new PersistentTasksCustomMetaData(
+            new NamedWriteableAwareStreamInput(out.bytes().streamInput(), getNamedWriteableRegistry()));
+        Set<String> expectedIds = new HashSet<>(tasks.getCurrentTaskIds());
+        expectedIds.remove("test_incompatible");


Same comment on the indirectness.

jasontedor · 2018-06-02T12:37:27Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexGraveyard.java

@@ -97,6 +95,11 @@ public String getWriteableName() {
        return TYPE;
    }

+    @Override
+    public Version getMinimalSupportedVersion() {


See my comment about littering.

jasontedor · 2018-06-02T12:37:34Z

server/src/main/java/org/elasticsearch/cluster/metadata/RepositoriesMetaData.java

@@ -103,6 +104,11 @@ public String getWriteableName() {
        return TYPE;
    }

+    @Override
+    public Version getMinimalSupportedVersion() {


See my comment about littering.

jasontedor · 2018-06-02T12:37:43Z

server/src/main/java/org/elasticsearch/ingest/IngestMetadata.java

@@ -69,6 +70,11 @@ public String getWriteableName() {
        return TYPE;
    }

+    @Override
+    public Version getMinimalSupportedVersion() {


See my comment about littering.

jasontedor · 2018-06-02T12:37:56Z

server/src/main/java/org/elasticsearch/script/ScriptMetaData.java

@@ -383,6 +383,11 @@ public String getWriteableName() {
        return TYPE;
    }

+    @Override
+    public Version getMinimalSupportedVersion() {


See my comment about littering.

jasontedor · 2018-06-02T12:38:35Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/LicensesMetaData.java

@@ -108,6 +108,11 @@ public String getWriteableName() {
        return TYPE;
    }

+    @Override
+    public Version getMinimalSupportedVersion() {


Don't mess with Texas.

bleskes · 2018-06-02T14:23:01Z

Thanks @jasontedor . I responded to all your comments. Can you take another look?

jasontedor · 2018-06-02T17:18:17Z

server/src/test/java/org/elasticsearch/persistent/PersistentTasksCustomMetaDataTests.java

@@ -299,8 +299,7 @@ public void testFeatureSerialization() throws IOException {

        PersistentTasksCustomMetaData read = new PersistentTasksCustomMetaData(
            new NamedWriteableAwareStreamInput(out.bytes().streamInput(), getNamedWriteableRegistry()));
-        Set<String> expectedIds = new HashSet<>(tasks.getCurrentTaskIds());
-        expectedIds.remove("test_incompatible");
+        Set<String> expectedIds = Collections.singleton("test_compatible");


Inline this directly to the assertion?

jasontedor · 2018-06-02T17:18:20Z

server/src/test/java/org/elasticsearch/persistent/PersistentTasksCustomMetaDataTests.java

        tasks.build().writeTo(out);

        final StreamInput input = out.bytes().streamInput();
        input.setVersion(streamVersion);
        PersistentTasksCustomMetaData read =
            new PersistentTasksCustomMetaData(new NamedWriteableAwareStreamInput(input, getNamedWriteableRegistry()));

-        Set<String> expectedIds = new HashSet<>(tasks.getCurrentTaskIds());
-        expectedIds.remove("test_incompatible_version");
+        Set<String> expectedIds = Collections.singleton("test_compatible_version");


Inline this directly to the assertion?

jasontedor

LGTM assuming CI is happy and you address the remaining 5.x versions that are less than the current minimum compatibility version.

bleskes · 2018-06-03T19:51:53Z

Thanks @jasontedor . I'll start the song and dance of backporting.

jasontedor · 2018-06-03T20:24:49Z

Thanks @bleskes.

With #31020 we introduced the ability for transport clients to indicate what features they support in order to make sure we don't serialize object to them they don't support. This PR adapts the serialization logic of persistent tasks to be aware of those features and not serialize tasks that aren't supported. Also, a version check is added for the future where we may add new tasks implementations and need to be able to indicate they shouldn't be serialized both to nodes and clients. As the implementation relies on the interface of `PersistentTaskParams`, these are no longer optional. That's acceptable as all current implementation have them and we plan to make `PersistentTaskParams` more central in the future. Relates to #30731

* master: Remove usage of explicit type in docs (elastic#29667) Share common readFrom/writeTo code in AcknowledgeResponse (elastic#30983) Adapt bwc versions after backporting elastic#31045 to 6.x Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (elastic#30989) Remove wrong link in index phrases doc Move pipeline APIs to ingest namespace (elastic#31027) [DOCS] Fixes accounting setting names (elastic#30863) [DOCS] Rewords _field_names documentation (elastic#31029) Index phrases (elastic#30450)

bleskes · 2018-06-04T14:22:10Z

This is now backported all the way to 6.3

* master: Match phrase queries against non-indexed fields should throw an exception (#31060) In the internal highlighter APIs, use the field type as opposed to the mapper. (#31039) [DOCS] Removes duplicated authentication pages Enable customizing REST tests blacklist (#31074) Make sure KeywordFieldMapper#clone preserves split_queries_on_whitespace. (#31049) [DOCS] Moves machine learning overview to stack-docs [ML] Add secondary sort to ML events (#31063) [Rollup] Specialize validation exception for easier management (#30339) Adapt bwc versions after backporting #31045 to 6.3 Remove usage of explicit type in docs (#29667) Share common readFrom/writeTo code in AcknowledgeResponse (#30983) Adapt bwc versions after backporting #31045 to 6.x Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (#30989) Remove wrong link in index phrases doc Move pipeline APIs to ingest namespace (#31027) [DOCS] Fixes accounting setting names (#30863) [DOCS] Rewords _field_names documentation (#31029) Index phrases (#30450) Remove leftover debugging from PTCMDT Fix PTCMDT#testMinVersionSerialization Make Persistent Tasks implementations version and feature aware (#31045)

Relates #31045

* 6.x: Add TRACE, CONNECT, and PATCH http methods (#31079) Change ObjectParser exception (#31030) Make sure KeywordFieldMapper#clone preserves split_queries_on_whitespace. (#31049) [DOCS] Removes duplicated authentication pages [Rollup] Specialize validation exception for easier management (#30339) Enable customizing REST tests blacklist (#31074) [DOCS] Moves machine learning overview to stack-docs [ML] Add secondary sort to ML events (#31063) QA: Check rollup job creation safety (#31036) Adapt bwc versions after backporting #31045 to 6.3 Remove usage of explicit type in docs (#29667) Move pipeline APIs to ingest namespace (#31027) Adapt bwc versions after backporting #31045 to 6.x Make Persistent Tasks implementations version and feature aware (#31045) Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (#30989) Remove wrong link in index phrases doc Reuse expiration date of trial licenses (#31033) Index phrases (#30450) [DOCS] Fixes accounting setting names (#30863) [DOCS] Rewords _field_names documentation (#31029)

Relates #31045

jasontedor and others added 18 commits May 31, 2018 22:37

Remove irrelevant lines from test

82ed4d4

Add comment on use of TreeSet

6d529fe

Remove unneeded parser in test

66f2ea9

Remove unneeded fields

25a861c

Guard feature de-serialization by version

0b39ce9

Add Javadocs

26071ff

Randomly add feature to test

8be176c

Change prefix of feature setting

1f66829

add version and feature support to persistent tasks

4b87ed8

add version and feature support to persistent tasks

aeee8ff

add version and feature support to persistent tasks

18a9a6f

unit tests

35463b3

Merge remote-tracking branch 'upstream/master' into persistent_tasks_…

dbc5076

…features

Merge remote-tracking branch 'upstream/master' into persistent_tasks_…

1a31b99

…features

require params

e6e5413

fix tests

5e6de98

Merge branch 'master' into persistent_tasks_features

e481804

bleskes added blocker :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. labels Jun 2, 2018

bleskes requested review from ywelsch and jasontedor June 2, 2018 11:55

jasontedor requested changes Jun 2, 2018

View reviewed changes

bleskes added 2 commits June 2, 2018 16:00

java docs

ce99bbe

feedback

1f83926

bleskes requested a review from jasontedor June 2, 2018 14:23

jasontedor reviewed Jun 2, 2018

View reviewed changes

jasontedor approved these changes Jun 3, 2018

View reviewed changes

bleskes added 3 commits June 3, 2018 17:53

v_5_4_0 back

2aa7a85

V_5.x -> minimum compat

ef015fa

fix testSnapshotDeletionsInProgressSerialization

307b714

bleskes merged commit a7ceefe into elastic:master Jun 3, 2018

bleskes added v7.0.0 backport pending v6.3.0 v6.4.0 labels Jun 3, 2018

bleskes deleted the persistent_tasks_features branch June 3, 2018 19:51

bleskes added a commit that referenced this pull request Jun 4, 2018

Adapt bwc versions after backporting #31045 to 6.x

5be554f

bleskes added a commit that referenced this pull request Jun 4, 2018

Adapt bwc versions after backporting #31045 to 6.x

ccb78c2

bleskes added a commit that referenced this pull request Jun 4, 2018

Adapt bwc versions after backporting #31045 to 6.3

1e5dc50

bleskes added a commit that referenced this pull request Jun 4, 2018

Adapt bwc versions after backporting #31045 to 6.3

b9b7bd4

bleskes removed the backport pending label Jun 4, 2018

bleskes added a commit that referenced this pull request Jun 4, 2018

Adapt bwc versions after backporting #31045 to 6.3

167b9b3

dnhatn added a commit that referenced this pull request Jun 4, 2018

Adapt changes in PersistentTaskParams

6ee6404

Relates #31045

dnhatn added a commit that referenced this pull request Jun 4, 2018

Adapt changes in PersistentTaskParams

d125719

Relates #31045

jasontedor mentioned this pull request Jun 5, 2018

Enable rolling upgrades from default distribution prior to 6.3.0 to default distribution post 6.3.0 #30731

Closed

9 tasks

jpountz added the >enhancement label Jun 13, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Persistent Tasks implementations version and feature aware #31045

Make Persistent Tasks implementations version and feature aware #31045

bleskes commented Jun 2, 2018

elasticmachine commented Jun 2, 2018

jasontedor left a comment

jasontedor Jun 2, 2018

jasontedor Jun 2, 2018

bleskes Jun 2, 2018

jasontedor Jun 2, 2018

bleskes Jun 3, 2018

bleskes Jun 3, 2018

jasontedor Jun 3, 2018

This comment was marked as resolved.

This comment was marked as resolved.

jasontedor Jun 2, 2018

bleskes Jun 2, 2018

jasontedor Jun 2, 2018

jasontedor Jun 2, 2018

jasontedor Jun 2, 2018

jasontedor Jun 2, 2018

jasontedor Jun 2, 2018

jasontedor Jun 2, 2018

bleskes commented Jun 2, 2018

jasontedor Jun 2, 2018

bleskes Jun 3, 2018

jasontedor Jun 2, 2018

bleskes Jun 3, 2018

jasontedor left a comment

bleskes commented Jun 3, 2018

jasontedor commented Jun 3, 2018

bleskes commented Jun 4, 2018

Make Persistent Tasks implementations version and feature aware #31045

Make Persistent Tasks implementations version and feature aware #31045

Conversation

bleskes commented Jun 2, 2018

elasticmachine commented Jun 2, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

bleskes commented Jun 3, 2018

jasontedor commented Jun 3, 2018

bleskes commented Jun 4, 2018