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

Netty: Add HTTP pipelining support #8299

Merged

Conversation

spinscale
Copy link
Contributor

This adds HTTP pipelining support to netty. Previously pipelining was not
supported due to the asynchronous nature of elasticsearch. The first request
that was returned by Elasticsearch, was returned as first response,
regardless of the correct order.

The solution to this problem is to add a handler to the netty pipeline
that maintains an ordered list and thus orders the responses before
returning them to the client. This means, we will always have some state
on the server side and also requires some memory in order to keep the
responses there.

Pipelining is enabled by default, but can be configured by setting the
http.pipelining property to true|false. In addition the maximum size of
the event queue can be configured.

The initial netty handler is copied from this repo
https://github.com/typesafehub/netty-http-pipelining

Closes #2665

private final ClientBootstrap clientBootstrap;

public NettyHttpClient() {
clientBootstrap = new ClientBootstrap(new NioClientSocketChannelFactory(newSingleThreadExecutor(), newSingleThreadExecutor()));;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be cached thread pool, the default constructor does the right thing here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, also fixed in the HttpPipelineHandlerTest

@kimchy
Copy link
Member

kimchy commented Oct 31, 2014

few comments, LGTM

@spinscale spinscale force-pushed the feature/issue-2665-netty-http-pipelining branch from d369a03 to d96c6dd Compare October 31, 2014 14:39
ESLoggerFactory.getLogger("### MZ LOGGER").info("GOT {}", message);
}

latch.countDown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it should not also decrement the CountDownLatch if an exception is caught in the SimpleChannelUpstreamHandler ?

Something like

new SimpleChannelUpstreamHandler() {
        @Override
        public void messageReceived(..) {
                ...
                latch.countDown();
        }
        @Override
        public void exceptionCaught(...) {
                latch.countDown();
        }

Just to be sure that the client does not hang indefintily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, will add

@spinscale spinscale force-pushed the feature/issue-2665-netty-http-pipelining branch from d96c6dd to 325a8aa Compare October 31, 2014 15:07
This adds HTTP pipelining support to netty. Previously pipelining was not
supported due to the asynchronous nature of elasticsearch. The first request
that was returned by Elasticsearch, was returned as first response,
regardless of the correct order.

The solution to this problem is to add a handler to the netty pipeline
that maintains an ordered list and thus orders the responses before
returning them to the client. This means, we will always have some state
on the server side and also requires some memory in order to keep the
responses there.

Pipelining is enabled by default, but can be configured by setting the
http.pipelining property to true|false. In addition the maximum size of
the event queue can be configured.

The initial netty handler is copied from this repo
https://github.com/typesafehub/netty-http-pipelining

Closes elastic#2665
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP Pipelining causes responses to mixed up.
5 participants