Skip to content
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

Refactor guice startup #7289

Merged
merged 1 commit into from Aug 19, 2014

Conversation

Projects
None yet
4 participants
@spinscale
Copy link
Member

commented Aug 15, 2014

  • Removed & refactored unused module code
  • Allowed to set transports programmatically

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

@spinscale spinscale added the review label Aug 15, 2014

@uboness

View changes

src/main/java/org/elasticsearch/transport/TransportModule.java Outdated
defaultTransportModule = LocalTransportModule.class;
protected void configure() {
if (configuredTransportService != null) {
bind(TransportService.class).to(configuredTransportService).asEagerSingleton();

This comment has been minimized.

Copy link
@uboness

uboness Aug 15, 2014

Contributor

I would add logging here indicating that we're overriding any transport service that the user might have configured?

@uboness

View changes

src/main/java/org/elasticsearch/transport/TransportModule.java Outdated
if (!TransportService.class.equals(transportService)) {
bind(TransportService.class).to(transportService).asEagerSingleton();
if (configuredTransport != null) {
bind(Transport.class).to(configuredTransport).asEagerSingleton();

This comment has been minimized.

Copy link
@uboness

uboness Aug 15, 2014

Contributor

here too?

@uboness

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2014

left a couple of comment, besides that LGTM

@javanna

View changes

src/main/java/org/elasticsearch/http/HttpServerModule.java Outdated
bind(HttpServer.class).asEagerSingleton();
}

public void setHttpServerTranspor(Class<? extends HttpServerTransport> httpServerTransport) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 15, 2014

Member

missing t at the end of the method name

@spinscale

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2014

added logging in all three cases as well forcing the user to specify the plugin responsible for the change

@uboness

View changes

src/main/java/org/elasticsearch/transport/TransportModule.java Outdated
}
}

public void setTransportService(Class<? extends TransportService> transportService, Plugin plugin) {

This comment has been minimized.

Copy link
@uboness

uboness Aug 15, 2014

Contributor

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)

@spinscale

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2014

switched back to use a string instead of a plugin as argument

@uboness

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2014

LGTM

@spinscale spinscale added the breaking label Aug 18, 2014

@uboness

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2014

why is this labeled as breaking?

@spinscale

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2014

@uboness change of the transport implementation does not require you to specify a module any more but the conrete transport, using the settings key http.type and transport.type, so upgrading requires a config change

Transport: Refactor guice startup
* 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

@spinscale spinscale merged commit 247ff7d into elastic:master Aug 19, 2014

@spinscale spinscale added v1.4.0 and removed review labels Aug 19, 2014

spinscale added a commit that referenced this pull request Aug 19, 2014

Transport: Refactor guice startup
* 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

javanna added a commit that referenced this pull request Aug 20, 2014

[TEST] UnicastBackwardsCompatibilityTest should not copy internal nod…
…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.

javanna added a commit that referenced this pull request Aug 20, 2014

[TEST] UnicastBackwardsCompatibilityTest should not copy internal nod…
…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.

spinscale added a commit that referenced this pull request Sep 8, 2014

Transport: Refactor guice startup
* 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

javanna added a commit that referenced this pull request Sep 8, 2014

[TEST] UnicastBackwardsCompatibilityTest should not copy internal nod…
…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 clintongormley changed the title Transport: Refactor guice startup Internal: Refactor guice startup Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Refactor guice startup Refactor guice startup Jun 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.