Remove PROTO-serialization from IndexMetaData.Custom by imotov · Pull Request #32749 · elastic/elasticsearch

imotov · 2018-08-09T15:29:51Z

Switches IndexMetaData.Custom to the standard named serialization.

Supersedes #32683

@jasontedor this is a version of #32683 without any breaking changes in terms of functionality and user interface. It will require small changes in existing plugins that use IndexMetaData.Custom, but otherwise, I think there is total parity with existing implementation minus PROTO-serialization.

@tlrx could you take a look at my (mis)use of SPI and XContent parsers in CreateIndexRequest and PutIndexTemplateRequest? Did I miss anything there?

Switches IndexMetaData.Custom to standard named serialization. Supersedes elastic#32683

elasticmachine · 2018-08-09T15:29:53Z

Pinging @elastic/es-core-infra

imotov · 2018-08-13T16:44:07Z

retest this please

tlrx · 2018-08-14T08:59:00Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

+                if (entry.getValue() instanceof Map) {
+                    if (customsRegistry == null) {
+                        // Custom registry wasn't specified - we are in client mode let's try loading it from SPI
+                        customsRegistry = new NamedXContentRegistry(getProvidedNamedXContents());


I'm not sure to understand what you want to achieve here (though I understand the proto based serialization removal).

I don't think we want a request object to create a NamedXContentRegistry with only some provided names xcontents as it won't contain all named xcontent parsers but only the ones that provide and implement NamedXContentProvider.

The NamedXContentRegistry is usually instantiated once on the server or client side and it must contain all required named xcontent parsers. This list is potentially enriched with named xcontent parsers provided by modules or plugins that implement NamedXContentProvider. But in any case, the NamedXContentRegistry is instantiated once on server or client side with everything it needs.

On server side the registry is instantiated in the Node class and in the RestHighLevelClient class for an example of client side instanciation.

In your case, I guess you'll have to plumb the NamedXContentRegistry down to where you want to parse this content. I think it's also possible to retrieve a ref on the current registry from a XContentParser instance.

Thanks! As per our discussion, I have removed the automatic NamedXContentRegistry discovery and replaced with passing it to source(...) method as parameters.

imotov · 2018-08-14T19:11:45Z

retest this please

dakrone

This LGTM, thanks Igor, I left a couple tiny comments.

@jasontedor might want to take a look and make sure this will be workable for CCR though

dakrone · 2018-08-16T21:27:35Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java

+     */
+    public <T> boolean supportsNamedObject(Class<T> categoryClass, String name) {
+        Map<String, Entry> parsers = registry.get(categoryClass);
+        if (parsers == null || registry.isEmpty()) {


I think the registry.isEmpty() check is superfluous here

dakrone · 2018-08-16T21:31:15Z

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

-            throw new IllegalArgumentException("No custom metadata prototype registered for type [" + type + "]");
+    public static Custom parseCustom(String name, Map<String, Object> map, DeprecationHandler deprecationHandler,
+                                     NamedXContentRegistry customsRegistry) throws IOException {
+        XContentBuilder customXContent = XContentFactory.jsonBuilder()


Can you put this in a try-with-resources block?

dakrone · 2018-08-16T21:32:58Z

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

+                            IndexMetaData.Custom custom = parser.namedObject(IndexMetaData.Custom.class, currentFieldName, null);
+                            builder.putCustom(custom.getWriteableName(), custom);
+                        } catch (UnknownNamedObjectException ex) {
+                            logger.warn("Skipping unknown custom index metadata object with type {}", currentFieldName);


I think this should be lowercase (suuuuper minor)

…indexmetadata-custom-2

dakrone · 2018-08-21T16:12:07Z

@jasontedor will this work for CCR's needs?

…ndexmetadata-custom-2

dakrone · 2018-08-30T03:07:42Z

Okay I've made some changes to this PR:

This PR nows removes the deprecated Custom class in IndexMetaData, in favor
of a Map<String, DiffableStringMap> that is used to store custom index
metadata. As part of this, there is now no way to set this metadata in a
template or create index request (since it's only set by plugins, or dedicated
REST endpoints).

The Map<String, DiffableStringMap> is intended to be a namespaced Map<String, String> (DiffableStringMap implements Map<String, String>, so the signature
is more like Map<String, Map<String, String>>). This is so we can do things
like:

Map<String, String> ccrMeta = indexMetaData.getCustom("ccr");

And then have complete control over the metadata. This also means any
plugin/feature that uses this has to manage its own BWC, as the map is just
serialized as a map. It also means that if metadata is put in the map that isn't
used (for instance, if a plugin were removed), it causes no failures the way
an unregistered Setting would.

The reason I use a custom DiffableStringMap here rather than a plain
Map<String, String> is so the map can be diffed with previous cluster state
updates for serialization.

/ping @martijnvg @jasontedor @s1monw as I believe you were all interested in the implementation of this

martijnvg

@dakrone This looks great! I left a couple of comments.

martijnvg · 2018-08-30T03:21:54Z

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

+ * This is a {@code Map<String, String>} that implements AbstractDiffable so it
+ * can be used for cluster state purposes
+ */
+public class DiffableStringMap extends AbstractDiffable<DiffableStringMap> implements Map<String, String> {


Can we extend from AbstractMap here instead of AbstractDiffable and then implement Diffable instead of Map? I think that would simplify this class and the only method that AbstractDiffable provides here is overwritten here.

Yep that definitely works, will do that

martijnvg · 2018-08-30T03:26:12Z

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

-    @SuppressWarnings("unchecked")
-    public <T extends Custom> T custom(String type) {
-        return (T) customs.get(type);
+    public Map<String, String> getCustomData(final String key) {


Are we returning here a mutable map? I don't think we should do that here. We should at least wrap it in Collections#unmodifiableMap()

Yep good idea, I'll do that

martijnvg · 2018-08-30T03:27:44Z

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

+        assertThat(dsm3.get("newkey"), equalTo("yay"));
+        assertThat(dsm3.get("baz"), equalTo("eggplant"));
+        assertThat(dsm3.get("potato"), equalTo(null));
+    }


Can we also add a test here with a few more maps and entries (maybe randomly generated) than in the existing test?

and maybe also a test that verifies that the serialization works?

I also wonder if we need a random test that adds randomly to one map and then passes it on as a diff to another and makes sure after we did that N times the maps are the same?

and maybe also a test that verifies that the serialization works?

The serialization was added as part of the IndexMetaDataTests, but I'll add one here for targeted serialization

I also wonder if we need a random test that adds randomly to one map and then passes it on as a diff to another and makes sure after we did that N times the maps are the same?

I'll add a test for that as well

s1monw

Looks good in general. I left some minors and once @martijnvg comments are resolved I am good to go with that. nice and simple!

s1monw · 2018-08-30T07:34:01Z

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

+ * This is a {@code Map<String, String>} that implements AbstractDiffable so it
+ * can be used for cluster state purposes
+ */
+public class DiffableStringMap extends AbstractDiffable<DiffableStringMap> implements Map<String, String> {


s1monw · 2018-08-30T07:36:11Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

+        if (in.getVersion().before(Version.V_7_0_0_alpha1)) {
+            // This used to be the size of custom metadata classes
+            int customSize = in.readVInt();
+            assert customSize == 0 : "unexpected custom metadata when none is supported";


hmm do we need to skip the size if we are in production? I mean that assert will not trip if we run without -ea

I'm not following, do you want it to be a regular exception instead of an assert? We still need to do the readVInt regardless of -ea or without, it's just the 0 size comparison that is an assert for tests

I think that we need to do a hard exception indeed, we won't be able to read the rest of the message at this point because we can't read the customs (no IndexMetaData#lookupPrototypeSafe anymore) when there are unexpected ones. That is, customSize > 0 is fatal for us.

s1monw · 2018-08-30T07:39:01Z

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

+        assertThat(dsm3.get("newkey"), equalTo("yay"));
+        assertThat(dsm3.get("baz"), equalTo("eggplant"));
+        assertThat(dsm3.get("potato"), equalTo(null));
+    }


I also wonder if we need a random test that adds randomly to one map and then passes it on as a diff to another and makes sure after we did that N times the maps are the same?

s1monw · 2018-08-30T07:39:33Z

server/src/test/resources/META-INF/services/org.elasticsearch.plugins.spi.NamedXContentProvider

@@ -1 +1 @@
-org.elasticsearch.plugins.spi.NamedXContentProviderTests$TestNamedXContentProvider
+org.elasticsearch.plugins.spi.NamedXContentProviderTests$TestNamedXContentProvider


this is an unrelated change?

Yep sorry, this was part of reverting Igor's changes that missed a newline

s1monw · 2018-08-30T07:40:02Z

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

+        m2.put("newkey", "yay");
+        m2.put("baz", "eggplant");
+        DiffableStringMap dsm2 = new DiffableStringMap(m2);
+        logger.info("--> 1: {}", dsm);


can we remove the logging in this unittest?

Sure, will do this

extends AbstractMap<String, String> and implement Diffable<String>

jasontedor

I left one comment.

jasontedor · 2018-08-30T17:30:31Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

+        if (in.getVersion().before(Version.V_7_0_0_alpha1)) {
+            // This used to be the size of custom metadata classes
+            int customSize = in.readVInt();
+            assert customSize == 0 : "unexpected custom metadata when none is supported";


I think that we need to do a hard exception indeed, we won't be able to read the rest of the message at this point because we can't read the customs (no IndexMetaData#lookupPrototypeSafe anymore) when there are unexpected ones. That is, customSize > 0 is fatal for us.

jasontedor · 2018-08-30T17:30:53Z

...c/main/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateRequest.java

+        if (in.getVersion().before(Version.V_7_0_0_alpha1)) {
+            // Used to be used for custom index metadata
+            int customSize = in.readVInt();
+            assert customSize == 0 : "expected not to have any custom metadata";


This should be fatal.

jasontedor · 2018-08-30T17:31:21Z

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

+                builder.putCustom(key, custom);
+            }
+        } else {
+            assert customSize == 0 : "expected no custom index metadata";


This should be fatal.

jasontedor · 2018-08-30T17:31:55Z

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

+        if (in.getVersion().before(Version.V_7_0_0_alpha1)) {
+            // Previously we allowed custom metadata
+            int customSize = in.readVInt();
+            assert customSize == 0 : "expected no custom metadata";


This should be fatal.

dakrone · 2018-08-30T17:42:15Z

Thanks @jasontedor, I've addressed your comments

jasontedor

LGTM.

…32749) This PR removes the deprecated `Custom` class in `IndexMetaData`, in favor of a `Map<String, DiffableStringMap>` that is used to store custom index metadata. As part of this, there is now no way to set this metadata in a template or create index request (since it's only set by plugins, or dedicated REST endpoints). The `Map<String, DiffableStringMap>` is intended to be a namespaced `Map<String, String>` (`DiffableStringMap` implements `Map<String, String>`, so the signature is more like `Map<String, Map<String, String>>`). This is so we can do things like: ``` java Map<String, String> ccrMeta = indexMetaData.getCustom("ccr"); ``` And then have complete control over the metadata. This also means any plugin/feature that uses this has to manage its own BWC, as the map is just serialized as a map. It also means that if metadata is put in the map that isn't used (for instance, if a plugin were removed), it causes no failures the way an unregistered `Setting` would. The reason I use a custom `DiffableStringMap` here rather than a plain `Map<String, String>` is so the map can be diffed with previous cluster state updates for serialization. Supersedes elastic#32683

martijnvg · 2018-08-31T01:06:43Z

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

+    }
+
+    @Override
+    public boolean equals(Object obj) {


I don't think it is necessary to overwrite equals(), hashCode() and toString() here? AbstractMap already implements these methods.

martijnvg · 2018-08-31T01:07:40Z

LGTM - left one minor comment.

* 6.x: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test TEST: Disable soft-deletes in ParentChildTestCase TEST: Disable randomized soft-deletes settings Integrates soft-deletes into Elasticsearch (#33222) drop `index.shard.check_on_startup: fix` (#32279) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi [DOCS] Moves ml folder from x-pack/docs to docs (#33248) TEST: mute more SmokeTestWatcherWithSecurityIT tests [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307) MINOR: Remove Dead Code from PathTrie (#33280) (#33306) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) Enable forbiddenapis server java9 (#33245) [MUTE] SmokeTestWatcherWithSecurityIT flaky tests Add region ISO code to GeoIP Ingest plugin (#31669) (#33276) Don't be strict for 6.x Update serialization versions for custom IndexMetaData backport Replace IndexMetaData.Custom with Map-based custom metadata (#32749) Painless: Fix Bindings Bug (#33274) SQL: prevent duplicate generation for repeated aggs (#33252) TEST: Mute testMonitorClusterHealth Fix serialization of empty field capabilities response (#33263) Fix nested _source retrieval with includes/excludes (#33180) [DOCS] TLS file resources are reloadable (#33258) Watcher: Ensure TriggerEngine start replaces existing watches (#33157) Ignore module-info in jar hell checks (#33011) Fix docs build after #33241 [DOC] Repository GCS ADC not supported (#33238) Upgrade to latest Gradle 4.10 (#32801) Fix/30904 cluster formation part2 (#32877) Move file-based discovery to core (#33241) HLRC: add client side RefreshPolicy (#33209) [Kerberos] Add unsupported languages for tests (#33253) Watcher: Reload properly on remote shard change (#33167) Fix classpath security checks for external tests. (#33066) [Rollup] Only allow aggregating on multiples of configured interval (#32052) Added deprecation warning for rescore in scroll queries (#33070) Apply settings filter to get cluster settings API (#33247) [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability (#32743) HLRC: create base timed request class (#33216) HLRC: Use Optional in validation logic (#33104) Painless: Add Bindings (#33042)

Remove PROTO-serialization from IndexMetaData.Custom (II)

3f5d717

Switches IndexMetaData.Custom to standard named serialization. Supersedes elastic#32683

imotov added :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. v7.0.0 >refactoring v6.5.0 labels Aug 9, 2018

imotov requested review from dakrone and tlrx August 9, 2018 15:29

tlrx reviewed Aug 14, 2018

View reviewed changes

Remove automatic xcontent registry discovery

b8239f5

dakrone approved these changes Aug 16, 2018

View reviewed changes

imotov added 2 commits August 20, 2018 11:39

Address @dakrone's review comments

9986710

Merge remote-tracking branch 'elastic/master' into remove-proto-from-…

d85bd10

…indexmetadata-custom-2

elasticmachine mentioned this pull request Aug 23, 2018

[meta] Index Lifecycle Management Plan #29823

Closed

Merge remote-tracking branch 'origin/master' into remove-proto-from-i…

290afc4

…ndexmetadata-custom-2

dakrone added the WIP label Aug 28, 2018

dakrone added 11 commits August 28, 2018 13:32

WIP

c1fc780

Merge remote-tracking branch 'origin/master' into remove-proto-from-i…

4a95041

…ndexmetadata-custom-2

Remove more vestiges

df0f86a

Fix some things

f4831f2

Fix a test

5a471f2

AwaitsFix a test until we decide

08d3a50

Remove most of the REST-facing custom metadata parts

b8c9628

More removals

db11af1

Merge remote-tracking branch 'origin/master' into remove-proto-from-i…

ea49b8c

…ndexmetadata-custom-2

Implement diffs for DiffableStringMap

cd46e81

Fix NativeRolesStoreTests

af115fd

Fix serialization for pre-7.x versions

cdb21f9

dakrone requested review from jasontedor, martijnvg and s1monw August 30, 2018 03:08

dakrone removed the WIP label Aug 30, 2018

martijnvg reviewed Aug 30, 2018

View reviewed changes

martijnvg added the review label Aug 30, 2018

s1monw approved these changes Aug 30, 2018

View reviewed changes

dakrone added 5 commits August 30, 2018 10:39

Simplify DiffableStringMap

65c8165

extends AbstractMap<String, String> and implement Diffable<String>

Return an immutable map from getCustomData(String)

2643723

Remove newline from SPI file

3d15e60

Remove logging from test

bf50d45

Add random diffing and serialization test

6523d40

jasontedor requested changes Aug 30, 2018

View reviewed changes

Make receiving custom metadata throw real exception

81be184

jasontedor approved these changes Aug 30, 2018

View reviewed changes

dakrone merged commit 001b78f into elastic:master Aug 30, 2018

martijnvg reviewed Aug 31, 2018

View reviewed changes

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

imotov deleted the remove-proto-from-indexmetadata-custom-2 branch May 1, 2020 22:28

jaymode mentioned this pull request Jun 15, 2020

Replace private index settings with index metadata #50758

Closed

		@@ -1 +1 @@
		org.elasticsearch.plugins.spi.NamedXContentProviderTests$TestNamedXContentProvider No newline at end of file
		org.elasticsearch.plugins.spi.NamedXContentProviderTests$TestNamedXContentProvider

Conversation

imotov commented Aug 9, 2018

Uh oh!

elasticmachine commented Aug 9, 2018

Uh oh!

imotov commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov commented Aug 14, 2018

Uh oh!

dakrone 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!

dakrone commented Aug 21, 2018

Uh oh!

dakrone commented Aug 30, 2018

Uh oh!

martijnvg 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!

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!

s1monw 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!

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!

jasontedor 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