Delegate toXContent logic from ClusterState to its member classes #48218 #52743

zacharymorn · 2020-02-25T07:17:04Z

This is a continuation of PR #48924, but to approach the problem in the recommended direction, as that PR has too many commits to cleanly show the changes.

MetaData.toXContent is already parameterized as it checks for API context for certain serializations. Hence I’ve organized the 4 commits in that PR as follows:

Add API and Gateway serialization tests for complex ClusterState and MetaData objects without modifying production code
Copy over ClusterState.toXContent serialization test metadata section to MetaData.toXContent API serialization tests to highlight the difference and setup baseline
Parameterize MetaData.toXContent further and have its serialization under API context to be as close as possible to ClusterState.
1. Note that as the updates to existing serialization test cases show, there are inconsistencies of serializations under API context.
2. We can pick and choose which serializaton scheme we actually want, and which one should no longer be supported under API context.
Replace ClusterState.toXContent metadata section with parameterized MetaData.toXContent

… to MetaData.toXContent serialization test

… Gateway context. Note that as the updates to existing serialization test cases show, there are inconsistencies of serializations under API context. We can pick and choose which serializaton scheme we actually want, and which one should no longer be supported under API context.

…etaData.toXContent

ywelsch · 2020-03-03T07:46:37Z

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


+        Map<String, String> mapParams = new HashMap<>();


can you add this as a static field to this class and call it API_PARAMS?

Sure will do.

ywelsch

Thanks for working on this. It's heading in the right direction. I've left some comments (in particular, I'm surprised that we need to change deserialization code).

ywelsch · 2020-03-03T07:48:51Z

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

+            }
+            else {
+                indexMetaData.getSettings()
+                    .toXContent(builder, new MapParams(Collections.singletonMap("flat_settings", params.param("flat_settings"))));


should we just pass on params instead of just extracting flat_settings?

As before the rest of params was not passed along to IndexMetaData.toXContent, I suspect this may change the serialization and thus introduce a breaking change. I can try and submit another commit with that to see what happen?

Actually the original ClusterState#toXContent uses the whole params

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

Line 444 in 2e617c3

settings.toXContent(builder, params);

. I've updated it.

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

ywelsch · 2020-03-03T08:01:54Z

server/src/test/java/org/elasticsearch/cluster/metadata/ToAndFromJsonMetaDataTests.java

@@ -246,13 +245,19 @@ public void testSimpleJsonFromAndTo() throws IOException {
        assertThat(indexMetaData.mapping().source().string(), equalTo(MAPPING_SOURCE1));
        assertThat(indexMetaData.getAliases().size(), equalTo(3));
        assertThat(indexMetaData.getAliases().get("alias1").alias(), equalTo("alias1"));
-        assertThat(indexMetaData.getAliases().get("alias1").filter().string(), equalTo(ALIAS_FILTER1));
+        // By default, MetaData.Builder.toXContent(metaData) will use ToXContent.EMPTY_PARAMS, which generates the same serialization


I think we should remove that static method (MetaData.Builder.toXContent) which seems to be only used by this test.

Instead, we should test both API and non-API serialization in this class, and non-API deserialization.

I've moved the new serialization tests from MetaDataTests to ToAndFromJsonMetaDataTests.

I've inlined / removed 1 MetaData.Builder#toXContent which is only used by test. There's another one though that takes more parameters and used in production code, so I left that one alone.

ywelsch · 2020-03-03T08:03:21Z

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

@@ -1318,7 +1360,21 @@ public static IndexMetaData fromXContent(XContentParser parser) throws IOExcepti
                                throw new IllegalArgumentException("Unexpected token: " + token);
                            }
                        }
-                    } else if ("warmers".equals(currentFieldName)) {
+                    } else if (KEY_PRIMARY_TERMS.equals(currentFieldName)) {


why did you add deserialization code here?

Is it because we are testing API deserialization somewhere? If so, I would rather have the tests changed to test non-API deserialization.

Same comment of other places below.

The primary reason for adding these is, the original ClusterState.toXContent emits array serialization for aliases and object serialization for primary_terms, which are different from what MetaData.toXContent emits, which is object serialization for aliases and array serialization for primary_terms, under API context. The delta can been seen here 072e05d#diff-4385d650a8cef0592db3454ad42e4bf3L1172-R1157

In general, as the duplicated code diverges, there are conflicting serializations under API context.

So the additional deserialization here is to be able to read back in the different serialization emitted by the new code (namely, array serialization for aliases and object serialization for primary_terms)

I think we should avoid being able to read back the API-serialized XContent. There is no requirement to do so, and it introduces leniency in the parsing here, with the possibility of silently hiding bugs where the wrong context is chosen for serialization.

I've given this a shot and removed deserialization for both primary_terms and aliases. These ended up breaking a lot of tests as they serialized under default context (API), and deserialized for further checking. I thus removed the breaking tests (as little as I can, but more can be removed as what's left are just setup code) and isolated the removal in this commit 0c11613.

If we don't want to remove these many tests for such small changes, the other possible approach is to not emit those different serialization for aliases and primary_terms for ClusterState.toXContent. This obviously would break code relying on it.

Please let me know which way you prefer, or if you have other ideas around this.

elasticmachine · 2020-03-11T12:56:20Z

Pinging @elastic/es-distributed (:Distributed/Distributed)

ywelsch

I've left more comments. Thanks for iterating with me on this.

ywelsch · 2020-03-23T09:11:10Z

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

-                    builder.endArray();
-                }
-                builder.endObject();
+        API_PARAMS.put("flat_settings", params.param("flat_settings"));


This mutates the global variable on every call? Certainly not what we want?

I've removed API_PARAMS as suggested so this is no longer relevant.

ywelsch · 2020-03-23T09:16:40Z

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

-            builder.endObject();
+        // meta data
+        if (metrics.contains(Metric.METADATA)) {
+            metaData.toXContent(builder, new ToXContent.MapParams(API_PARAMS));


Instead of passing down a limited set of parameters here, I wonder if we should pass all of them, but additionally inject the API context.

Given that Metadata is already using API context as default, I wonder if we need to do anything here in this class. Perhaps it's sufficient to just pass down the params as is?

That's a good point. The delegation refactoring ultimately do away the need for selecting params in this layer. I've removed it.

ywelsch · 2020-03-23T09:20:16Z

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

@@ -1318,7 +1360,21 @@ public static IndexMetaData fromXContent(XContentParser parser) throws IOExcepti
                                throw new IllegalArgumentException("Unexpected token: " + token);
                            }
                        }
-                    } else if ("warmers".equals(currentFieldName)) {
+                    } else if (KEY_PRIMARY_TERMS.equals(currentFieldName)) {


I think we should avoid being able to read back the API-serialized XContent. There is no requirement to do so, and it introduces leniency in the parsing here, with the possibility of silently hiding bugs where the wrong context is chosen for serialization.

ywelsch · 2020-03-23T09:23:06Z

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

-            builder.endObject();
-            return Strings.toString(builder);
-        }
-
        public static void toXContent(MetaData metaData, XContentBuilder builder, ToXContent.Params params) throws IOException {
            XContentContext context = XContentContext.valueOf(params.param(CONTEXT_MODE_PARAM, "API"));


Use CONTEXT_MODE_API constant here?

ywelsch · 2020-03-23T09:53:44Z

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

            builder.field("cluster_uuid", metaData.clusterUUID);
+            // This is required for some serialization under API context, as some tests in MetaDataTests assert its presenc


I'm not sure I understand this comment? Is it outdated?

Yes sorry. This was added in between the commits, as was meant to say that ClusterState.toXContent will now start to emit cluster_uuid_committed in its serialization, as I retried to remove cluster_uuid_committed before to keep ClusterState.toXContent serialization unchanged, but had to put back due to the test asserting it.

ywelsch · 2020-03-23T09:55:00Z

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

@@ -1347,7 +1345,8 @@ public static MetaData fromXContent(XContentParser parser, boolean preserveUnkno
                currentFieldName = parser.currentName();
            }

-            if (!"meta-data".equals(parser.currentName())) {
+            if (!"meta-data".equals(parser.currentName()) &&
+                !"metadata".equals(parser.currentName())) {


As commented above, let's remove the lenient parsing here.

Done. This also caused some test cases to be broken and removed as they relied on deserialization of API-serialized XContent 0c11613

ywelsch · 2020-03-23T09:57:17Z

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

+                            builder.putAlias(AliasMetaData.builder(parser.text()).build());
+                        }
+                    }
+                    else {


move these elses up (lot's of other occurrences in this file)

I've removed these changes as they are also for reading back API-serialized XContent. I did correct else placement in other places though.

… params from ClusterState to MetaData

…s well as the tests that get broken

zacharymorn · 2020-03-24T03:11:45Z

I've left more comments. Thanks for iterating with me on this.

Sure no problem. I do see these changes to be prone to errors and bugs, and so very appreciate your patience and time for reviewing them carefully!

Once these changes are in good shape, I'll merge in master and resolve conflicts as well.

ywelsch · 2020-03-24T09:52:58Z

@elasticmachine ok to test

ywelsch · 2020-03-24T16:12:22Z

@elasticmachine test this please

zacharymorn · 2020-03-25T01:31:12Z

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

@@ -1451,7 +1452,13 @@ public static Settings addHumanReadableSettings(Settings settings) {
        return builder.build();
    }

-    private static final ToXContent.Params FORMAT_PARAMS = new MapParams(Collections.singletonMap("binary", "true"));
+    private static final ToXContent.Params FORMAT_PARAMS;


Didn't realize we could use this to save the tests, good to know!

ywelsch

LGTM

…stic#48218 (elastic#52743) Delegates the toXContent serialization logic from the ClusterState class to its member classes, making the code in MetaData, IndexMetaData, ... aware of the serialization context (API vs GATEWAY). This makes it clearer what the differences are between API and GATEWAY serialization. Long-term, both should be unified. Co-authored-by: Yannick Welsch <yannick@welsch.lu>

…4192) Backport of #52743 Co-authored-by: Yannick Welsch <yannick@welsch.lu>

Realigns master with 7.x

zacharymorn added 4 commits February 19, 2020 09:54

Add serialization tests for ClusterState and MetaData toXContent methods

422dadb

Copy over ClusterState.toXContent serialization test metadata section…

072e05d

… to MetaData.toXContent serialization test

Substitue ClusterState.toXContent metadata serialization logic with M…

b021eac

…etaData.toXContent

zacharymorn mentioned this pull request Feb 25, 2020

Delegate toXContent logic from ClusterState to its member classes #48218 #48924

Closed

ywelsch self-requested a review March 3, 2020 07:43

ywelsch reviewed Mar 3, 2020

View reviewed changes

ywelsch self-requested a review March 11, 2020 12:55

ywelsch added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >non-issue labels Mar 11, 2020

Address comments

958d41a

ywelsch suggested changes Mar 23, 2020

View reviewed changes

zacharymorn added 2 commits March 23, 2020 16:37

Address comments to place else on the same line, and pass down entire…

089d7a3

… params from ClusterState to MetaData

Remove lenient deserialization parsing for API-serialized XContent, a…

0c11613

…s well as the tests that get broken

ywelsch added 5 commits March 24, 2020 08:35

Fix tests

977a428

Merge remote-tracking branch 'elastic/master' into pr/52743

608d627

fix merge issue

ae97011

fix tests

db90041

undo more changes

76564f9

ywelsch added 5 commits March 24, 2020 11:57

Fix alias filter parsing and test

7a30a36

Remove transient settings

e99f60a

always print indices in API mode

b1eb05a

Merge remote-tracking branch 'elastic/master' into pr/52743

cc4f317

checkstyle

3fe6cea

ywelsch self-requested a review March 24, 2020 13:49

checkstyle

7235ade

zacharymorn commented Mar 25, 2020

View reviewed changes

ywelsch added 5 commits March 25, 2020 09:05

Merge remote-tracking branch 'elastic/master' into pr/52743

af8286d

More test fixing

6560116

one more test to fix after merging master

8c7a308

more fixes yet

8567d91

another fix

51b31b4

ywelsch approved these changes Mar 25, 2020

View reviewed changes

ywelsch merged commit dc0d35e into elastic:master Mar 25, 2020

ywelsch added v7.7.0 v8.0.0 v7.8.0 v7.7.1 backport pending and removed v7.7.0 labels Mar 25, 2020

ywelsch mentioned this pull request Mar 25, 2020

Delegate toXContent logic from ClusterState to its member classes #54192

Merged

ywelsch removed v7.7.1 backport pending labels Mar 25, 2020

ywelsch added a commit that referenced this pull request Mar 26, 2020

Delegate toXContent logic from ClusterState to its member classes (#5…

8176f1c

…4192) Backport of #52743 Co-authored-by: Yannick Welsch <yannick@welsch.lu>

ywelsch added a commit that referenced this pull request Mar 26, 2020

Adaptations after backport of #52743

827820d

Realigns master with 7.x

williamrandolph mentioned this pull request Jan 7, 2021

ClusterState's toXContent (imperfectly) duplicates logic from other classes' toXContent #48218

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegate toXContent logic from ClusterState to its member classes #48218 #52743

Delegate toXContent logic from ClusterState to its member classes #48218 #52743

zacharymorn commented Feb 25, 2020

ywelsch Mar 3, 2020

zacharymorn Mar 4, 2020

zacharymorn Mar 20, 2020

ywelsch left a comment

ywelsch Mar 3, 2020

zacharymorn Mar 4, 2020

zacharymorn Mar 20, 2020

ywelsch Mar 3, 2020

zacharymorn Mar 20, 2020

ywelsch Mar 3, 2020

zacharymorn Mar 4, 2020

ywelsch Mar 23, 2020

zacharymorn Mar 24, 2020

elasticmachine commented Mar 11, 2020

ywelsch left a comment

ywelsch Mar 23, 2020

zacharymorn Mar 24, 2020

ywelsch Mar 23, 2020

zacharymorn Mar 24, 2020

ywelsch Mar 23, 2020

ywelsch Mar 23, 2020

zacharymorn Mar 24, 2020

ywelsch Mar 23, 2020

zacharymorn Mar 24, 2020

ywelsch Mar 23, 2020

zacharymorn Mar 24, 2020

ywelsch Mar 23, 2020

zacharymorn Mar 24, 2020

zacharymorn commented Mar 24, 2020

ywelsch commented Mar 24, 2020

ywelsch commented Mar 24, 2020 •

edited

Loading

zacharymorn Mar 25, 2020

ywelsch left a comment

		builder.field("cluster_uuid", metaData.clusterUUID);
		// This is required for some serialization under API context, as some tests in MetaDataTests assert its presenc

Delegate toXContent logic from ClusterState to its member classes #48218 #52743

Delegate toXContent logic from ClusterState to its member classes #48218 #52743

Conversation

zacharymorn commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 11, 2020

ywelsch 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

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

zacharymorn commented Mar 24, 2020

ywelsch commented Mar 24, 2020

ywelsch commented Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch commented Mar 24, 2020 •

edited

Loading