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

Introduce Netty 4 #19526

Merged
merged 24 commits into from
Jul 23, 2016
Merged

Introduce Netty 4 #19526

merged 24 commits into from
Jul 23, 2016

Conversation

jasontedor
Copy link
Member

This commit adds transport-netty4, a transport and HTTP implementation
based on Netty 4.

Closes #3226

@jasontedor jasontedor added review :Distributed/Network Http and internode communication implementations >upgrade v5.0.0-alpha5 labels Jul 21, 2016
@jasontedor
Copy link
Member Author

The full top-level gradle check passes locally with a custom-built snapshot of Netty 4 but the CI builds are going to fail until a 4.1.4.Final-SNAPSHOT trickles into the repositories that contains some of the latest fixes in Netty. However, the review cycle can start but we should off on a master integration until a 4.1.4.Final release is cut.

No attempts have been made to optimize, this can happen in follow ups. Further, no attempts have been made to eliminate any code duplication between transport-netty3 and transport-netty4. Only one of these implementations is going to survive, we shouldn't bind them together to only unbind them later when we kill one of the implementations.

if (SETTING_CORS_ALLOW_CREDENTIALS.get(settings)) {
builder.allowCredentials();
}
String[] strMethods = settings.getAsArray(SETTING_CORS_ALLOW_METHODS.getKey());
Copy link

Choose a reason for hiding this comment

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

This change will need to be ported over here: #19522

Copy link
Member Author

@jasontedor jasontedor Jul 21, 2016

Choose a reason for hiding this comment

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

Thanks for the heads up, but I want that to be in a follow up. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #19562 for tracking.

@s1monw
Copy link
Contributor

s1monw commented Jul 21, 2016

I did one pass, didn't look too close to the http impls maybe @spinscale can look at pipelining and cors etc.

@jasontedor
Copy link
Member Author

@s1monw Thanks. I've responded to your first round of feedback.

@Override
protected Object decode(ChannelHandlerContext ctx, Channel channel, ChannelBuffer buffer) throws Exception {
try {
boolean continueProcessing = TcpTransport.validateMessageHeader(Netty3Utils.toBytesReference(buffer));
boolean continueProcessing = transport.validateMessageHeader(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also be a static method aagain I guess

Today when an exception occurs on the transport layer, an exception
bubbles up where it is caught in the Netty layer and wrapped in
Netty-specific exceptions. These exceptions must be unwrapped to be
pushed back down to the transport layer for handling. This commit cleans
up this unwrapping in the Netty 4 module.
This commit updates the SHAs for the Netty 4 dependencies to the SHAs
for the 20160721-*-6 snapshots.
This commit renames the TransportServiceAdapter#received and
TransportServiceAdapter#sent interface methods for clarity, and
clarifies the usage in Netty4MessageChannelHandler.
This commit improves the handling of resources in the Netty 4 networking
implementation.
To determine if we want to close a connection on completion of a
request, we need compare the value of the connection header to the
string "keep-alive" when the connection is an HTTP 1.0
connection. However, the conditional under question here uses the unary
not operator which can sometimes be tricky to see, especially in long
expressions. This commit replaces the uses of the unary not operator
with comparison to false.
Today we initialize the params field for an instance of
Netty4HttpRequest in its constructor. It might be simpler to just
initialize the field directly, so that's what we do in this commit.
The base REST request implementation RestRequest defines some abstract
methods that are just duplicated across the implementations of the
inheriting classes. This commit moves these duplicated impelementations
to the base class for simplification.
This commit marks Netty 3 as the default transport client networking
implementation, for now.
On the transport layer when decoding the header, we catch and silently
swallow and IllegalStateException. This is because this state naturally
arises when we have fully consumed the incoming byte buffer. This commit
adds a comment explaining this situation.
This commit fixes a broken test case in BytesRestResponseTests that
was broken by an earlier refactoring.
This commit addresses a resource leak in the Netty 4 module that arises
when the content of a request is held on to after the request has
completed.
This commit updates the SHAs for the Netty 4 dependencies to the SHAs
for the 20160721-*-7 snapshots.
This commit fixes an NPE that arises in the retry tests due to the
ordering of the setup methods.
This commit modifes the Netty 4 build so that the packaged REST tests
are run with the Netty 4 module integration tests.
This commit reverts changes to size header frame decoding to keep the
API simple, for now.
This commit modifies the Netty 4 dependencies to use Elastic-built
artifacts until an official release of Netty 4.1.4.Final+ is made. This
is so that we can rely on a stable artifact in the CI environment.
This commit renames a lambda future variable from future1 to f because
it's cleaner that way.
This commit clarifies the conversion from BytesReference to
CompositeBuffer in Netty4Utils, and provides a safer conversion.
This commit updates the SHAs for the Netty 4 dependencies to the SHAs
for the elastic-20160722-*-7 snapshots.
This commit adds netty.assert.buglevel as an additional setting in the
client.
This commit reverts the renaming of client:transport back from
client:transport-netty.
This commit preserves headers from the thread context on Netty 4 HTTP
responses.
@jasontedor jasontedor merged commit 2d1b058 into elastic:master Jul 23, 2016
@jasontedor jasontedor deleted the transport-netty-4 branch July 23, 2016 02:26
@normanmaurer
Copy link

@jasontedor if you want I can have a look at your netty related code

@jasontedor
Copy link
Member Author

@normanmaurer Sure! Note there's still a lot of work left to do here, but any feedback is welcome and appreciated. I'm on vacation starting today until August 1, so I won't read/reply until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >upgrade v5.0.0-alpha5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants