New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugins: Removed plugin.types #13055

Merged
merged 4 commits into from Aug 30, 2015

Conversation

Projects
None yet
4 participants
@rjernst
Member

rjernst commented Aug 22, 2015

The setting plugin.types is currently used to load plugins from the
classpath. This is necessary in tests, as well as the transport client.

This change removes the setting, and replaces it with the ability to
directly add plugins when building a transport client, as well as
infrastructure in the integration tests to specify which plugin classes
should be loaded on each node.

Note: I have nocommit in the docs as I'm not sure how to properly create a link to another page.

Plugins: Removed plugin.types
The setting `plugin.types` is currently used to load plugins from the
classpath. This is necessary in tests, as well as the transport client.

This change removes the setting, and replaces it with the ability to
directly add plugins when building a transport client, as well as
infrastructure in the integration tests to specify which plugin classes
should be loaded on each node.
@clintongormley

This comment has been minimized.

Member

clintongormley commented Aug 24, 2015

The link can be written as:

read more in {ref}/integration-tests.html#changing-node-configuration[Changing Node Configuration].
@rjernst

This comment has been minimized.

Member

rjernst commented Aug 24, 2015

I've updated the link and synced with master.

@dakrone

This comment has been minimized.

Member

dakrone commented Aug 25, 2015

@rjernst the way this is specified for the MockNode seems a little crazy, can we do a var-args version of MockNode so we can do:

public MockNode(Settings settings, boolean loadConfigSettings, Version version, Class<? extends Plugin>... classpathPlugins) {
    ...
}

And then call it with:

MockNode m = new MockNode(settings, false, Version.CURRENT, NativeScriptPlugin.class, MyOtherPlugin.class);
@rjernst

This comment has been minimized.

Member

rjernst commented Aug 26, 2015

@dakrone There are only 3 callers really: test cluster start, test restart node, and transport client. They all already have a collection (because they've been building it up in a list). The calls that you seem to be referring to are just for those 3 script benchmarks...so I don't think we should switch to an array because it would actually be more work in the "real" callers.

@@ -80,6 +82,11 @@ private Settings nodeSettings(String dataPath) {
.build();
}
@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return pluginList(MockTransportService.TestPlugin.class);

This comment has been minimized.

@dakrone

dakrone Aug 26, 2015

Member

Hmm.. I see the downside for this being that with this the plugin is used suite-wide instead of for the single test as before?

This comment has been minimized.

@rjernst

rjernst Aug 26, 2015

Member

Does that matter here? The test would have already got this plugin randomized in anyways. At first I started trying to add in the ability to set it also on a per node basis, but there was way too much overhead to make it worthwhile (so many versions of create(async)node to pass it through. We could always move the test that needs it out to its own suite if it is a real concern?

This comment has been minimized.

@dakrone

dakrone Aug 26, 2015

Member

Not really, just want to make sure I understood the behavior

@@ -636,7 +643,7 @@ public void registrationFailureTest() {
@Test
public void testThatSensitiveRepositorySettingsAreNotExposed() throws Exception {
Settings nodeSettings = settingsBuilder().put("plugin.types", MockRepository.Plugin.class.getName()).build();
Settings nodeSettings = settingsBuilder().put().build();

This comment has been minimized.

@dakrone

dakrone Aug 26, 2015

Member

Can be Settings.EMPTY?

@dakrone

This comment has been minimized.

Member

dakrone commented Aug 26, 2015

Okay, the 3 callers thing makes sense, I think this looks good and makes the tests with custom plugins look much nicer!

rjernst added a commit that referenced this pull request Aug 30, 2015

@rjernst rjernst merged commit fc84040 into elastic:master Aug 30, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@rjernst rjernst deleted the rjernst:tell_me_your_plugins branch Aug 30, 2015

@clintongormley clintongormley added v2.1.0 and removed v2.0.0 labels Aug 31, 2015

imotov added a commit to imotov/elasticsearch-native-script-example that referenced this pull request Aug 31, 2015

}
/** Helper method to create list of plugins without specifying generic types. */
protected static Collection<Class<? extends Plugin>> pluginList(Class<? extends Plugin>... plugins) {

This comment has been minimized.

@robinst

robinst Dec 21, 2015

Contributor

This method should have the @SafeVarargs annotation, otherwise every use of it will get a warning.

This comment has been minimized.

@rjernst

rjernst Dec 22, 2015

Member

@robinst This is already the case on master (see https://github.com/elastic/elasticsearch/blob/master/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java), which was done as part of a large warnings cleanup there. While that warnings cleanup is unlikely to be backported, would you like to open a PR to backport this fix?

This comment has been minimized.

@robinst

robinst Dec 26, 2015

Contributor

Should have checked master :). I don't think it's important enough for backporting, having it in a future release is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment