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

Refactoring to make MessageChannelHandler extensible #6915

Merged
merged 1 commit into from Jul 18, 2014

Conversation

Projects
None yet
6 participants
@spinscale
Copy link
Member

spinscale commented Jul 18, 2014

Small refactorings to make the MessageChannelHandler more extensible.
Also allowed access to the different netty pipelines

This is the fix after the first version had problems with the HTTP
transport due to wrong reusing channel handlers, which is the reason
why tests failed. This version now uses its own ChannelPipelineFactory
again.

Relates #6889 (which had been reverted in the first place)

@spinscale spinscale added the review label Jul 18, 2014

@uboness

This comment has been minimized.

Copy link
Contributor

uboness commented Jul 18, 2014

LGTM

@kimchy

View changes

src/main/java/org/elasticsearch/http/netty/NettyHttpServerTransport.java Outdated
requestDecoder.setMaxCumulationBufferCapacity(Integer.MAX_VALUE);
} else {
requestDecoder.setMaxCumulationBufferCapacity((int) transport.maxCumulationBufferCapacity.bytes());
public ChannelPipelineFactory configureServerChannelPipelineFactory() {

This comment has been minimized.

Copy link
@kimchy

kimchy Jul 18, 2014

Member

can the implementation still be a static factory class that is returned?

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Jul 18, 2014

LGTM

@spinscale

This comment has been minimized.

Copy link
Member Author

spinscale commented Jul 18, 2014

created static classes and added tests, should be ready now

@uboness

View changes

src/main/java/org/elasticsearch/http/netty/NettyHttpServerTransport.java Outdated

private final NettyHttpServerTransport transport;
private static class HttpChannelPipelineFactory implements ChannelPipelineFactory {

This comment has been minimized.

Copy link
@uboness

uboness Jul 18, 2014

Contributor

can you make it protected

@uboness

View changes

src/main/java/org/elasticsearch/transport/netty/NettyTransport.java Outdated
return new ClientChannelPipelineFactory(this);
}

private static class ClientChannelPipelineFactory implements ChannelPipelineFactory {

This comment has been minimized.

Copy link
@uboness

uboness Jul 18, 2014

Contributor

protected?

@uboness

View changes

src/main/java/org/elasticsearch/transport/netty/NettyTransport.java Outdated
return new ServerChannelPipeFactory(this);
}

private static class ServerChannelPipeFactory implements ChannelPipelineFactory {

This comment has been minimized.

Copy link
@uboness

uboness Jul 18, 2014

Contributor

protected?

@uboness

This comment has been minimized.

Copy link
Contributor

uboness commented Jul 18, 2014

few comments, other than that LGTM

@uboness

This comment has been minimized.

Copy link
Contributor

uboness commented Jul 18, 2014

LGTM

Netty: Refactoring to make MessageChannelHandler extensible
Small refactorings to make the MessageChannelHandler more extensible.
Also allowed access to the different netty pipelines

This is the fix after the first version had problems with the HTTP
transport due to wrong reusing channel handlers, which is the reason
why tests failed.

Relates #6889
Closes #6915

@spinscale spinscale merged commit 1816951 into elastic:master Jul 18, 2014

spinscale added a commit that referenced this pull request Jul 18, 2014

Netty: Refactoring to make MessageChannelHandler extensible
Small refactorings to make the MessageChannelHandler more extensible.
Also allowed access to the different netty pipelines

This is the fix after the first version had problems with the HTTP
transport due to wrong reusing channel handlers, which is the reason
why tests failed.

Relates #6889
Closes #6915
@mattweber

This comment has been minimized.

Copy link
Contributor

mattweber commented Jul 18, 2014

Awesome, this should make it possible for someone to write the SSL transport support as a plugin. I had investigated going down this route a while back but got stuck because of the shading of the Netty jar. It tossed a bunch of the Netty code that was not used in ES but needed if I wanted to add the SSL support.

@jpountz jpountz removed the review label Jul 24, 2014

@clintongormley clintongormley changed the title Netty: Refactoring to make MessageChannelHandler extensible Internal: Refactoring to make MessageChannelHandler extensible Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Refactoring to make MessageChannelHandler extensible Refactoring to make MessageChannelHandler extensible Jun 7, 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.