Refactor guice startup #7289
Refactor guice startup #7289
Conversation
| defaultTransportModule = LocalTransportModule.class; | ||
| protected void configure() { | ||
| if (configuredTransportService != null) { | ||
| bind(TransportService.class).to(configuredTransportService).asEagerSingleton(); |
There was a problem hiding this comment.
I would add logging here indicating that we're overriding any transport service that the user might have configured?
|
left a couple of comment, besides that LGTM |
| bind(HttpServer.class).asEagerSingleton(); | ||
| } | ||
|
|
||
| public void setHttpServerTranspor(Class<? extends HttpServerTransport> httpServerTransport) { |
There was a problem hiding this comment.
missing t at the end of the method name
|
added logging in all three cases as well forcing the user to specify the plugin responsible for the change |
| } | ||
| } | ||
|
|
||
| public void setTransportService(Class<? extends TransportService> transportService, Plugin plugin) { |
There was a problem hiding this comment.
While I understand why passing Plugin here is safe, after thinking about it a bit, I think I prefer replacing the Plugin plugin with String source to give the flexibility to choose whether this logic should be applied on the Plugin itself (using onModule or processModules) or on a different PreProcessModule (that latter feels more natural to me)
|
switched back to use a string instead of a plugin as argument |
|
LGTM |
|
why is this labeled as breaking? |
|
@uboness change of the transport implementation does not require you to specify a module any more but the conrete transport, using the settings key |
* Removed & refactored unused module code * Allowed to set transports programmatically * Allow to set the source of the changed transport Note: The current implementation breaks BWC as you need to specify a concrete transport now instead of a module if you want to use a different Transport or HttpServerTransport Closes elastic#7289
* Removed & refactored unused module code * Allowed to set transports programmatically * Allow to set the source of the changed transport Note: The current implementation breaks BWC as you need to specify a concrete transport now instead of a module if you want to use a different Transport or HttpServerTransport Closes #7289
…e settings to external nodes Recent test failures triggered by #7289 were caused by this, simply because internal node settings (transport type key) that are not supported by the external older nodes were copied to them by mistake.
…e settings to external nodes Recent test failures triggered by #7289 were caused by this, simply because internal node settings (transport type key) that are not supported by the external older nodes were copied to them by mistake.
* Removed & refactored unused module code * Allowed to set transports programmatically * Allow to set the source of the changed transport Note: The current implementation breaks BWC as you need to specify a concrete transport now instead of a module if you want to use a different Transport or HttpServerTransport Closes #7289
…e settings to external nodes Recent test failures triggered by #7289 were caused by this, simply because internal node settings (transport type key) that are not supported by the external older nodes were copied to them by mistake.
clintongormley
changed the title
clintongormley
changed the title
Note: The current implementation breaks BWC as you need to specify a concrete
transport now instead of a module if you want to use a different
Transport or HttpServerTransport