-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Netty: Add HTTP pipelining support #8299
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
Conversation
private final ClientBootstrap clientBootstrap; | ||
|
||
public NettyHttpClient() { | ||
clientBootstrap = new ClientBootstrap(new NioClientSocketChannelFactory(newSingleThreadExecutor(), newSingleThreadExecutor()));; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
few comments, LGTM |
d369a03
to
d96c6dd
Compare
ESLoggerFactory.getLogger("### MZ LOGGER").info("GOT {}", message); | ||
} | ||
|
||
latch.countDown(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, will add
d96c6dd
to
325a8aa
Compare
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
325a8aa
to
5eeac2f
Compare
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