Simplify Plugin API for constructing modules #12952
Conversation
rjernst
added
>breaking
:Core/Infra/Plugins
The Plugin interface currently contains 6 different methods for adding modules. Elasticsearch has 3 different levels of injectors, and for each of those, there are two methods. The first takes no arguments and returns a collection of class objects to construct. The second takes a Settings object and returns a collection of module objects already constructed. The settings argument is unecessary because the plugin can already get the settings from its constructor. Removing that, the only difference between the two versions is returning an already constructed Module, or a module Class, and there is no reason the plugin can't construct all their modules themselves. This change reduces the plugin api down to just 3 methods for adding modules. Each returns a Collection<Module>. It also removes the processModule method, which was unnecessary since onModule implementations fullfill the same requirement. And finally, it renames the modules() method to nodeModules() so it is clear these are created once for each node.
rjernst
force-pushed
the
construct_it_yourself
branch
from
c34513d
to
2bf8459
Compare
|
I really like the change. Something I'm just wondering. I mean that before the change plugin components were loaded only when used. For example, |
|
@dadoonet This does not change anything about component loading. Components are still returned as classes and constructed with the injector. This is simply about modules, whose sole purpose is to bind classes the injector will use when necessary. |
|
@rjernst Thanks for confirming! |
|
I really like this change!. Two notes:
|
|
@kimchy Plugins have the option of a ctor that takes Settings. They can store this, and pass to any of their modules that need settings. |
|
Ok, settings are needed for the shard/index modules, since those settings are the index settings, not the node settings. I'll add that parameter back. |
|
Yea, it is the shard/index level Settings that are important... . I would still think how to clean up the duplication of which Settings to use, thinking that maybe cleanest is to pass the right Settings to all callbacks (modules/service) and not rely on black magic constructor based Settings injection, which one needs to remember are node based settings. But that is a different change. |
|
@kimchy I merged AbstractPlugin and Plugin, and added back Settings for the index/shard modules methods. |
| public Settings additionalSettings() { | ||
| return Settings.Builder.EMPTY_SETTINGS; | ||
| } | ||
| public abstract class AbstractPlugin extends Plugin { |
There was a problem hiding this comment.
do we really need this class?
There was a problem hiding this comment.
Ooops, meant to remove that.
|
added one comment other than that LGTM |
Simplify Plugin API for constructing modules
|
I will backport to the 2.0 branch once the beta is out. |
The Plugin interface currently contains 6 different methods for adding modules. Elasticsearch has 3 different levels of injectors, and for each of those, there are two methods. The first takes no arguments and returns a collection of class objects to construct. The second takes a Settings object and returns a collection of module objects already constructed. The settings argument is unecessary because the plugin can already get the settings from its constructor. Removing that, the only difference between the two versions is returning an already constructed Module, or a module Class, and there is no reason the plugin can't construct all their modules themselves. This change reduces the plugin api down to just 3 methods for adding modules. Each returns a Collection<Module>. It also removes the processModule method, which was unnecessary since onModule implementations fullfill the same requirement. And finally, it renames the modules() method to nodeModules() so it is clear these are created once for each node. Backport of #12952
The Plugin interface currently contains 6 different methods for
adding modules. Elasticsearch has 3 different levels of injectors,
and for each of those, there are two methods. The first takes no
arguments and returns a collection of class objects to construct. The
second takes a Settings object and returns a collection of module
objects already constructed. The settings argument is unecessary because
the plugin can already get the settings from its constructor. Removing
that, the only difference between the two versions is returning an
already constructed Module, or a module Class, and there is no reason
the plugin can't construct all their modules themselves.
This change reduces the plugin api down to just 3 methods for adding
modules. Each returns a Collection. It also removes the
processModule method, which was unnecessary since onModule
implementations fullfill the same requirement. And finally, it renames
the modules() method to nodeModules() so it is clear these are created
once for each node.