Better extension point for custom cluster state parts #21243

martijnvg · 2016-11-01T15:57:37Z

This adds a better extension point for custom cluster state parts by only allowing adding custom cluster state parts via the ClusterPlugin interface.

This is a WIP and would be great to get feedback.

By piggybacking on the named writeable infrastructure we can serialize custom cluster state parts for the internal protocol. This seems to workout nice as the serialization code doesn't need to know about custom cluster metadata implementations.
For the xcontent serialization there is no such thing as named writaebles, so I passed down a registry holding prototypes for the custom cluster state parts on all the places we need to do de-serialization. We can maybe consider adding something like named writable on top of XContentParser? However passing down the registry doesn't seem to be too bad.
Should custom index metadata be removed? It isn't used in any place. I think it should, but in a follow up change.
This change needs tests.

PR for #20888

nik9000

I had a quick scan. I left a few "why" style things but I think this is the right approach here.

I like the idea of making a NamedWriteableRegistry-like thing for xcontent but it isn't clear to me that we know how to get it to apply well across the board. I don't want us to make some nice general abstraction and only use it for cluster state. If we made such a thing it'd be an empty victory if we didn't move queries and aggs to it.

So I think the right thing to do is to get this in and then use the lessons learned to open a followup for the more generalized abstraction. A migration of that size deserves to be done in many PRs with more time for review. Something like this we want sooner I think.

nik9000 · 2016-11-07T12:43:44Z

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

This change looks properly BWC to me. ++

nik9000 · 2016-11-07T12:45:28Z

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

I feel like this should be a parameter to the source method. That way it isn't so hidden. It can be @Nullable and we can have javadoc explaining what it is for and when it is needed. I'm not sure if this source method is part of the public Java API or not because we really don't mark things so both everything is and nothing is. Anyway, I think it'd be fine to add a nullable parameter there.

Is the reason you are considering a "named writeable to xcontent" so we don't have to have these registry objects? I think that is a fine idea! I think some things won't work as they stand now but could be fixed. See things like QueryParseContext.

A fine idea that we can work on later, just to clear.

I feel like this should be a parameter to the source method.

Initially I didn't want change any method signatures, but it isn't a big deal after all as like you said just null can be provided. To make it even better we can add an overloaded version of the method that just accepts source.

Is the reason you are considering a "named writeable to xcontent" so we don't have to have these registry objects?

Yes, here and also in other places too.

nik9000 · 2016-11-07T12:47:13Z

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

I think we should throw an exception if the proto doesn't resolve to anything.

You made a Safe method for this already I think.

agreed, we should do that here. I'll use the Safe variants for that.

nik9000 · 2016-11-07T12:53:11Z

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

Same comments as class above.

nik9000 · 2016-11-07T12:54:02Z

core/src/main/java/org/elasticsearch/cluster/ClusterModule.java

Add javadoc?

nik9000 · 2016-11-07T13:09:32Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java

I think the author of the parameter list meant to do one per line.

nik9000 · 2016-11-07T13:14:13Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java

I'd remove this note. We expect that only custom cluster state named writeables are used here but we support any named writeables. While we don't use this now we don't actively prevent using it and a plugin might use it.

I think you meant to remove the note but didn't? I'm not sure.

nik9000 · 2016-11-07T13:16:25Z

core/src/main/java/org/elasticsearch/plugins/ClusterPlugin.java

Needs javadoc eventually.

nik9000 · 2016-11-07T13:19:06Z

core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

s/cRegister/pRegistry/`?

nik9000 · 2016-11-07T13:21:38Z

core/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java

nik9000 · 2016-11-07T13:31:51Z

And, yes, this looks like it maintains wire format and cluster state backwards compatibility to me. I wonder if we should add more stuff to qa:rolling-upgrade or some other test.

martijnvg · 2016-11-07T14:27:07Z

I wonder if we should add more stuff to qa:rolling-upgrade or some other test.

I'm not sure if this requires its own qa module or can be included in the qa:rolling-upgrade module. It would require a custom plugin and that would make the existing qa module more difficult as it tries to test other things too. I guess the best way to create rolling upgrade test for custom cluster state parts is to build a 5.0.x data dir and a 5.1.x plugin that is able to read it then the test can after upgrading check some assertions.

nik9000 · 2016-11-07T14:33:42Z

I guess the best way to create rolling upgrade test for custom cluster state parts is to build a 5.0.x data dir and a 5.1.x plugin that is able to read it then the test can after upgrading check some assertions.

OldIndexBackwardsCompatibilityIT does the whole "old index version" thing. Maybe that is a good thing to build on?

martijnvg · 2016-11-07T15:54:35Z

OldIndexBackwardsCompatibilityIT does the whole "old index version" thing. Maybe that is a good thing to build on?

I think that using OldIndexBackwardsCompatibilityIT would work.

martijnvg · 2016-11-09T11:50:22Z

@nik9000 I've updated the PR.

nik9000 · 2016-11-09T14:43:10Z

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

Maybe instead of supporting a null registry we can provide an empty one instead? That feels like it'd be cleaner.

nik9000 · 2016-11-09T16:37:03Z

core/src/main/java/org/elasticsearch/cluster/ClusterModule.java

nik9000 · 2016-11-09T16:40:19Z

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

I'm not a fan of this null support either. I'd prefer sending an empty registry over null.

nik9000 · 2016-11-09T16:48:08Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java

I think you meant to remove the note but didn't? I'm not sure.

nik9000 · 2016-11-09T16:49:11Z

core/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java

Can you fix this javadoc? Those @param annotations are fairly useless.

nik9000 · 2016-11-09T16:54:06Z

...rc/test/java/org/elasticsearch/bwcompat/CustomClusterStatePartsBackwardsCompatibilityIT.java

Can you add some javadoc to explain what this does and some docs to explain why we need @ESIntegTestCase.ClusterScope and @LuceneTestCase.SuppressFileSystems?

Where does the file come from? I think we need to commit the script that generates it so we can make it when we release the next version, right?

I don't know how to automate this in a script, since it involves as plugin that specifies custom index metadata.

This file contains the code that creates the data directory and contains the plugin written on the 5.0.0 codebase: https://gist.github.com/martijnvg/ff48df064ad60ee983ec4fd7c7471a4c

Just chatted with Nik and I'll remove the custom plugin in test that is used the test custom index metadata. Then the test is simpler and it can be inlined in OldIndexBackwardsCompatibilityIT class and allows scripting.

nik9000 · 2016-11-09T17:02:20Z

...rc/test/java/org/elasticsearch/bwcompat/CustomClusterStatePartsBackwardsCompatibilityIT.java

This'll fail the build.

ouch, silly IDE

nik9000 · 2016-11-09T17:03:56Z

I think we're getting a longer and longer list of things that we need to run on upgrade. Maybe we need to start a running list somewhere....

nik9000 · 2016-11-09T17:09:15Z

...rc/test/java/org/elasticsearch/bwcompat/CustomClusterStatePartsBackwardsCompatibilityIT.java

Can you also add a test starting a node that doesn't have the plugin installed? The node should start, I think. Though I don't know what it should do with the data that it no longer understands. I think we have a setting like required_plugins to guard against accidental removal of a plugin so we can assume that any removal of the plugin is intentional. I think....

Good point. I'll add that test.

I agree that the required_plugins setting is an extra guard, but if that setting is forgotten there is nothing stopping the node from starting up.

Yeah! I think it is right to start the node though, right? You have to be able to uninstall plugins if you want, right?

That is true, but that wasn't your intention, you can lose the custom part in the cluster state.

…by only allowing adding custom cluster, metadata and index metadata via the ClusterPlugin interface. * By piggybacking on the named writeable infrastructure when can serialize custom cluster metadata for the internal protocol. This seems to workout nice as the serialization code doesn't need to know about custom cluster metadata implementations. * For the xcontent serialization there is no such thing as named writaebles, so I passed down a registry holding prototypes for custom metadate on all the places we need to de-serialize custom metadata. We can maybe consider adding something like named writable on top of XContentParser? However passing down the registry doesn't seem to be too bad. * Custom index metadata doesn't work yet. I think we can remove it as it isn't used in any place. Not sure what others think about that, but that can be done in another PR. * This change needs tests.

bleskes · 2016-11-10T11:21:23Z

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

this is existing behavior but it's bad... forgetting to install a plugin immediately implies the deletion of it's state (and solving it will be tricky). Can we add this warn log and open an issue to deal with it? (maybe the plugin manager to should remove custom meta when uninstalling. not sure)

I've opened #21471

bleskes · 2016-11-10T11:23:35Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java

            DiscoverySettings discoverySettings,
-            ClusterName clusterName) {
+            ClusterName clusterName,
+            CustomPrototypeRegistry registry) {


I wonder if we should pass these registries all over the place? maybe it's better to give a cluster diff and cluster state deserializers ? It doesn't feel like PublishClusterStateAction should care about the registry.

The main reason I needed to provide CustomPrototypeRegistry here is because in the handleIncomingClusterStateRequest(...) method (line 380-385) it creates a new StreamInput instance and have to wrap that with a NamedWriteableAwareStreamInput instance.

I might be missing something - but do we need both the NamedWriteable in the stream and the explicit registry when reading a diff? Diff<ClusterState> diff = lastSeenClusterState.readDiffFrom(in, registry);

but do we need both the NamedWriteable in the stream

Yes, otherwise nodes that receive a new cluster state don't know how the read the custom cluster state parts. (in the synchronized block line 388 till 407)

and the explicit registry when reading a diff?

Ideally we wouldn't need to pass the registry to the diff, but with the way it reads the custom diff map, I didn't see another of doing that without breaking bwc. (see here, here and here)

@martijnvg and I discussed this and Martijn is work on centralizing this code so all deserialization logic will be in one place. He's also looking into how it should be done if bwc was not a concern so we know how this should look going forward

@bleskes Like we discussed last friday I tried to stop using named writeables for cluster state related stuff and use the registry directly (so that all code will follow the same deserialization logic), but I ran into an issue that I don't know how to solve without creating a big mess. There are a couple of places where we serialize cluster related stuff in the action framework:

ClusterRerouteResponse#readFrom(...)

ClusterStateResponse#readFrom(...)

CreateIndexRequest#readFrom(...)

PutIndexTemplateRequest#readFrom(...)

In these readFrom() methods I don't have access to the registry. I don't see a way of passing down the registry without changing the Streamable interface.

@bleskes now that it seems that #21552 doesn't need this change, maybe we should work on adding named writable support to XContentParser and DiffableValueSerializer, so that this gets in a clean and consistent way.

bleskes · 2016-11-10T11:27:16Z

core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java

                    // sometimes the request comes in before the local node processed that cluster state
                    // in such cases we can load it from disk
-                    metaData = IndexMetaData.FORMAT.loadLatestState(logger, nodeEnv.indexPaths(shardId.getIndex()));
+                    metaData = metaStateService.getIndexMetadatFormat().loadLatestState(logger, nodeEnv.indexPaths(shardId.getIndex()));


maybe just make this a public method on metaStateService? (i.e., loadIndexLatestState) it already has nodeEnv and everything it needs. And now that I check it - it already has one :D org.elasticsearch.gateway.MetaStateService#loadIndexState

+1 good point :)

bleskes · 2016-11-10T11:28:46Z

core/src/main/java/org/elasticsearch/plugins/ClusterPlugin.java

+     * @return Custom cluster state parts used to store temporal information in the cluster state that doesn't need to
+     *         be persisted
+     */
+    default Collection<ClusterState.Custom> getCustomClusterState() {


nite: this needs a better names - getCustomParts? or getClusterStateCostums?

+1 for getClusterStateCostums()

…x metadata in create index request from `getIndexMetadataPrototypeSafe()` back to `getIndexMetadataPrototype()` as there is some logic in the request that demotes custom index metadata to index settings.

martijnvg · 2016-12-27T09:35:11Z

Superseded by #22336

martijnvg added WIP :Core/Infra/Plugins Plugin API and infrastructure labels Nov 1, 2016

martijnvg force-pushed the custom_cluster_metadata_plugin branch 4 times, most recently from fab869c to aaae041 Compare November 3, 2016 09:59

clintongormley added the >enhancement label Nov 3, 2016

martijnvg force-pushed the custom_cluster_metadata_plugin branch from aaae041 to 5e70b64 Compare November 7, 2016 12:55

nik9000 reviewed Nov 7, 2016

View reviewed changes

martijnvg force-pushed the custom_cluster_metadata_plugin branch from 5e70b64 to 96ad8b8 Compare November 9, 2016 11:49

martijnvg added review and removed WIP labels Nov 9, 2016

nik9000 requested changes Nov 9, 2016

View reviewed changes

martijnvg added 4 commits November 10, 2016 11:30

Make custum index matadata work and removed NOCOMMITS.

0989378

added tests and applied feedback

c3f8d74

Fix smaller feedbakc

e7c31be

martijnvg force-pushed the custom_cluster_metadata_plugin branch from 96ad8b8 to e7c31be Compare November 10, 2016 11:18

bleskes reviewed Nov 10, 2016

View reviewed changes

fixed compile and checkstyle errors, also changed looking custom inde…

7000b06

…x metadata in create index request from `getIndexMetadataPrototypeSafe()` back to `getIndexMetadataPrototype()` as there is some logic in the request that demotes custom index metadata to index settings.

martijnvg mentioned this pull request Nov 10, 2016

Forgot to install a plugin can cause custom cluster state parts to be removed #21471

Open

martijnvg added v5.1.1 v6.0.0-alpha1 labels Nov 11, 2016

Simplified backwards test

f191788

martijnvg removed the v5.1.1 label Nov 21, 2016

martijnvg closed this Dec 27, 2016

clintongormley removed the v6.0.0-alpha1 label Dec 28, 2016

Better extension point for custom cluster state parts #21243

Better extension point for custom cluster state parts #21243

Uh oh!

Conversation

martijnvg commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

nik9000 commented Nov 7, 2016

Uh oh!

martijnvg commented Nov 7, 2016

Uh oh!

nik9000 commented Nov 7, 2016

Uh oh!

martijnvg commented Nov 7, 2016

Uh oh!

martijnvg commented Nov 9, 2016

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!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Nov 1, 2016 •

edited

Loading