Move tribe to a module #25778
Move tribe to a module #25778
Conversation
ywelsch
added
:Core/Infra/Plugins
ywelsch
force-pushed
the
enhance/move-tribe-to-plugin
branch
from
e039323
to
92b07cd
Compare
I think a module is fine here we should have done the move to a plugin earlier we are late in to the game here. |
| @@ -920,8 +908,23 @@ protected SearchService newSearchService(ClusterService clusterService, IndicesS | |||
| return customNameResolvers; | |||
| } | |||
|
|
|||
| /** Constructs an internal node used as a client into a cluster fronted by this tribe node. */ | |||
| protected Node newTribeClientNode(Settings settings, Collection<Class<? extends Plugin>> classpathPlugins, Path configPath) { | |||
| public static class NodeBuilder { | |||
There was a problem hiding this comment.
do we really need this additional class? it's seem like users can provide all the stuff at once to the newNode method?
There was a problem hiding this comment.
I don't think we need this entire construct we can just subclass Node.java in the plugin / module and create a node via this?
There was a problem hiding this comment.
To play nicely with the MockNode concept used by the tests, we cannot just create the Node instance ourselves. However, by removing Guice injection, I've just replaced this class by a BiFunction<Settings, Path>.
| @@ -218,17 +155,31 @@ public static Settings processSettings(Settings settings) { | |||
|
|
|||
| private final NamedWriteableRegistry namedWriteableRegistry; | |||
|
|
|||
| public TribeService(Settings settings, Path configPath, ClusterService clusterService, final String tribeNodeId, | |||
| NamedWriteableRegistry namedWriteableRegistry, BiFunction<Settings, Path, Node> clientNodeBuilder) { | |||
| @Inject | |||
There was a problem hiding this comment.
I strongly believe we should not add any new @Inject anywhere in the codebase. we need to find a different way to do this
There was a problem hiding this comment.
I tried to unguice it in 2025150
|
|
||
| @Override | ||
| public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses() { | ||
| return Collections.singleton(TribeService.class); |
There was a problem hiding this comment.
why do we need to initialize it via guice, is there anything we can do about this? I think this is a blocker if we are adding inject annotations anywhere
| @@ -527,6 +517,10 @@ protected Node(final Environment environment, Collection<Class<? extends Plugin> | |||
| } | |||
| } | |||
|
|
|||
| public void addOnStartedListener(Runnable runnable) { | |||
| onStartedListeners.add(runnable); | |||
There was a problem hiding this comment.
our plugin API on the node level is pull based not push. Can we add a method to pull these from ClusterPlugin.java that way we are consistent and we fully control who calls this method.
| NamedXContentRegistry xContentRegistry) { | ||
| NamedXContentRegistry xContentRegistry, Environment environment, | ||
| NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry, | ||
| BiFunction<Settings, Path, Node> clientNodeBuilder) { |
There was a problem hiding this comment.
I think we should not make this a first class citizen. Instead I suggest we remove the Node#newNode entirely and add a MockTribePlugin class that passes such a function that delegates to / creates a MockNode. Given that tribe goes away I think that is the right tradeoff. WDYT?
|
Thanks @s1monw |
This commit moves tribe to a module, stripping core from the tribe functionality.
This commit moves tribe to a module, stripping core from the tribe functionality.
Companion PR to elastic#25778
This PR moves tribe to a module, stripping core from the tribe functionality. If desired, we could even make tribe a plugin instead of a module.