Allow plugins to upgrade global custom metadata on startup #19962

areek · 2016-08-11T17:50:08Z

Currently plugins can not inspect or upgrade custom
meta data on startup. This commit allow plugins
to check and/or upgrade global custom meta data on startup.
Plugins can stop a node if any custom meta data is not supported.

pickypg · 2016-08-11T18:04:08Z

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

+     * Returns a function that upgrades metadata on startup.
+     * Plugins should return the input metadata if no upgrade is required
+     * and throw {@link IllegalStateException} to stop a node from starting
+     * when unsupported metadata is found.


I suggest more official Javadoc:

/** * Provides a function to modify the metadata on startup. * <p> * Plugins should return the input metadata via {@link Function#identity()} if no upgrade is required. * @return Never {@code null}. The same or upgraded {@code MetaData}. * @throws IllegalStateException if the node needs to be stopped because {@code MetaData} is unsupported */

Thanks for the docs, added :)

pickypg · 2016-08-11T18:06:58Z

The change looks good, but we should add some tests to ensure it consistently happens.

rjernst · 2016-08-11T18:18:38Z

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


    @Nullable
    private volatile MetaData previousMetaData;

    private volatile Set<Index> previouslyWrittenIndices = emptySet();

    @Inject
-    public GatewayMetaState(Settings settings, NodeEnvironment nodeEnv, MetaStateService metaStateService,
+    public GatewayMetaState(Settings settings, NodeEnvironment nodeEnv, PluginsService pluginsService, MetaStateService metaStateService,


This is not the way to get the custom upgraders passed around (we should not be passing around PluginService). See other examples in Node: you should collect all the upgraders, and put them into a container that will then be injected (and eventually when GatewayMetaState is deguiced, we will just have it there as an arg, instead of via injection).

jasontedor · 2016-08-11T18:22:01Z

This pull request is missing tests. Some possible test cases:

What if two plugins upgrade?
What happens if the metadata is upgraded twice?

rjernst · 2016-08-11T18:26:12Z

Regarding two plugins upgrade, the plugins should not be modifying the same parts of metadata (this is really for custom metadata). Another suggested test is to check upgrading actually works (ie is it called at all for plugins which add an upgrader).

jasontedor · 2016-08-11T18:27:47Z

Regarding two plugins upgrade, the plugins should not be modifying the same parts of metadata (this is really for custom metadata).

Yes, but a test should check that both pieces of custom metadata are updated when two plugins upgrade on startup.

areek · 2016-08-12T18:57:15Z

Thanks @pickypg, @jasontedor & @rjernst for the feedback, I have restructured the code and added unit tests upgrading. Could you take another look?

Another suggested test is to check upgrading actually works (ie is it called at all for plugins which add an upgrader).

I wonder, if we need an explicit test for this in light of the current changes? We collect the functions just like we collect the namedWritables and components

rjernst · 2016-08-12T20:31:40Z

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

+     * <p>
+     * Plugins should return the input metadata via {@link Function#identity()} if no upgrade is required.
+     * @return Never {@code null}. The same or upgraded {@code MetaData}.
+     * @throws IllegalStateException if the node needs to be stopped because {@code MetaData} is unsupported


I would say something like "the node should not start" instead of the part about stopping (since this is during startup, while stopping implies the node is already started).

rjernst · 2016-08-12T20:41:23Z

Thanks @areek, I left some more comments. I think the tests should actually be on GatewayMetaState. And I think there may be a subtle bug regarding that: we need to be careful in there about only loading the metastate from disk once, passing it through all of the methods to manipulate/check it, and then writing it once all checks have passed. Otherwise, for example, right now index upgrading would happen, we write the new state, the plugged in metadata converters run, and if one fails, the node cannot be downgraded.

areek · 2016-08-16T16:28:44Z

@rjernst Thanks for pointing out that node cannot be downgraded, if plugins error out on upgrade. I have separated out logic to create upgraded meta data and writing them to disk. Now plugins can validate MetaData and upgrade MetaData.Customs on startup. I made the upgrade specific to MetaData.Custom as I don't think plugins should be upgrading anything except for the customs in the meta state (at least not on startup!), while plugins should be able to prevent startup upon finding a "too-old" meta data. Additionally, meta data upgrading became more unit-testable :) WDYT?

rjernst · 2016-08-16T16:31:20Z

@areek I think you misunderstood what I meant. I was suggesting GatewayMetaState should be modified, so that it reads the metastate at the beginning, runs it through all modifications/checks, and then writes it at the end of the ctor. I don't think we need two different interfaces for upgrade/validation. We just need to be more careful in the class calling all of these.

areek · 2016-08-16T16:47:46Z

I was suggesting GatewayMetaState should be modified, so that it reads the metastate at the beginning, runs it through all modifications/checks, and then writes it at the end of the ctor

@rjernst this is what it does. Now, all the upgrading logic is in GatewayMetaState.upgradeMetaData. The reason for not exposing upgrading the entire meta data by plugins is that it makes it hard to figure what part of the meta data exactly got upgraded and makes the upgrade code complex, as it deals with upgrading index meta data already. Also, I don't see why we need plugins to be able to upgrade anything other than the customs on startup.

I agree that you can still do the validation on Plugin#getCustomMetaDataUpgrader instead of having a separate Plugin#getMetaDataValidator, but IMO, it is more readable separating the validation from the upgrade and this allows plugin to validate based on the entire meta data while upgrading only customs.

rjernst · 2016-08-16T17:18:36Z

I don't like the validation separate because it doesn't match the purpose of the upgrade step. There should be no "validation only" possible. Think of it as upgrading is happening and it either succeeds with new meta state written, or fails with the node shutting down.

areek · 2016-08-16T17:25:22Z

hmm, ok, so we should do the validation in Plugin#getCustomMetaDataUpgrader then? I don't like exposing the entire meta data to plugins to upgrade.

rjernst · 2016-08-16T17:29:23Z

Yes, all I mean is to have one method on Plugin.

areek · 2016-08-16T17:41:39Z

I removed Plugin#getMetaDataValidator in the last iteration. @rjernst WDYT?

rjernst · 2016-08-16T18:05:41Z

core/src/test/java/org/elasticsearch/gateway/GatewayMetaStateTests.java

+                customs -> {
+                    List<MetaData.Custom> customList = new ArrayList<>();
+                    for (MetaData.Custom custom : customs) {
+                        if (custom.type().equals(CustomMetaData1.TYPE)) {


I notice this pattern in every implementation. Perhaps this should be a Map instead of Collection (keyed by the custom type name)? Then the map can be copied, and keys replaced, removed, or added easily, without needing to have logic for the other custom metadata that the plugin does not care about.

rjernst · 2016-08-16T18:08:02Z

Thanks @areek. It is looking better. I left a couple more suggestions.

areek · 2016-08-16T20:00:13Z

Thanks @rjernst for the suggestions, addressed all your feedback.

rjernst · 2016-08-16T20:05:54Z

LGTM

Currently plugins can not inspect or upgrade custom meta data on startup. This commit allow plugins to check and/or upgrade global custom meta data on startup. Plugins can stop a node if any custom meta data is not supported.

areek added >enhancement review :Core/Infra/Plugins Plugin API and infrastructure v5.0.0-beta1 labels Aug 11, 2016

pickypg reviewed Aug 11, 2016
View reviewed changes

rjernst reviewed Aug 11, 2016
View reviewed changes

areek force-pushed the enhancement/plugins_upgrade_hook branch 2 times, most recently from 57186d5 to d99bc98 Compare August 12, 2016 18:38

rjernst reviewed Aug 12, 2016
View reviewed changes

areek force-pushed the enhancement/plugins_upgrade_hook branch 2 times, most recently from e9139ad to ee06b4a Compare August 16, 2016 04:27

areek force-pushed the enhancement/plugins_upgrade_hook branch from ee06b4a to 4236c0f Compare August 16, 2016 17:37

areek changed the title ~~Allow plugins to upgrade metadata on startup~~ Allow plugins to upgrade global custom metadata on startup Aug 16, 2016

areek force-pushed the enhancement/plugins_upgrade_hook branch from 4236c0f to f5f2811 Compare August 16, 2016 17:40

areek force-pushed the enhancement/plugins_upgrade_hook branch from f5f2811 to 4c34896 Compare August 16, 2016 17:45

rjernst reviewed Aug 16, 2016
View reviewed changes

areek force-pushed the enhancement/plugins_upgrade_hook branch from 4c34896 to 97b2546 Compare August 16, 2016 19:57

areek force-pushed the enhancement/plugins_upgrade_hook branch from 97b2546 to c353ba9 Compare August 16, 2016 20:02

Allow plugins to upgrade global custom metadata on startup

75d4a9f

Currently plugins can not inspect or upgrade custom meta data on startup. This commit allow plugins to check and/or upgrade global custom meta data on startup. Plugins can stop a node if any custom meta data is not supported.

areek force-pushed the enhancement/plugins_upgrade_hook branch from c353ba9 to 75d4a9f Compare August 16, 2016 20:25

areek merged commit a61257e into elastic:master Aug 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow plugins to upgrade global custom metadata on startup #19962

Allow plugins to upgrade global custom metadata on startup #19962

areek commented Aug 11, 2016 •

edited

pickypg Aug 11, 2016

areek Aug 12, 2016

pickypg commented Aug 11, 2016

rjernst Aug 11, 2016

jasontedor commented Aug 11, 2016

rjernst commented Aug 11, 2016

jasontedor commented Aug 11, 2016

areek commented Aug 12, 2016

rjernst Aug 12, 2016

rjernst commented Aug 12, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

Allow plugins to upgrade global custom metadata on startup #19962

Allow plugins to upgrade global custom metadata on startup #19962

Conversation

areek commented Aug 11, 2016 • edited

pickypg Aug 11, 2016

Choose a reason for hiding this comment

areek Aug 12, 2016

Choose a reason for hiding this comment

pickypg commented Aug 11, 2016

rjernst Aug 11, 2016

Choose a reason for hiding this comment

jasontedor commented Aug 11, 2016

rjernst commented Aug 11, 2016

jasontedor commented Aug 11, 2016

areek commented Aug 12, 2016

rjernst Aug 12, 2016

Choose a reason for hiding this comment

rjernst commented Aug 12, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst Aug 16, 2016

Choose a reason for hiding this comment

rjernst commented Aug 16, 2016

areek commented Aug 16, 2016

rjernst commented Aug 16, 2016

areek commented Aug 11, 2016 •

edited