Netty version update 3.9 -> 4.1 by esevastyanov · Pull Request #29 · metamx/http-client

esevastyanov · 2017-06-04T14:43:01Z

Netty was improved a lot from 3.9 to 4.1
3.x -> 4.0 http://netty.io/wiki/new-and-noteworthy-in-4.0.html
4.0 -> 4.1 http://netty.io/wiki/new-and-noteworthy-in-4.1.html

Reduced GC pressure
Reduced memory bandwidth consumption
New codecs and handlers

# Conflicts: # pom.xml

drcrallen · 2017-06-05T03:45:12Z

Should do a major version update

drcrallen · 2017-06-05T03:46:04Z

On a side note, I'm excited to try this out: https://github.com/netty/netty/tree/4.1/codec-http2

logarithm · 2017-06-05T15:36:14Z

Our last tries showed that netty 4.1 is incompatible with spark 2.1.0. Could you please make sure, that this http-client implementation will work correctly with netty 4.0.x and 4.1.x in run time.

drcrallen · 2017-06-05T16:28:17Z

@logarithm do we have a record of what the problem was?

logarithm · 2017-06-05T16:40:18Z

@drcrallen Which record do you mean? We have commit in our private repo which fixes it. Problem was that spark is using 4.0.29 netty version, but in runtime we had 4.1. and spark was failing with exception.

drcrallen · 2017-06-05T17:17:51Z

related: https://issues.apache.org/jira/browse/SPARK-19552 and apache/spark#16888

leventov · 2017-06-05T19:44:08Z

pom.xml

-            <artifactId>netty</artifactId>
-            <version>3.10.6.Final</version>
+            <artifactId>netty-all</artifactId>
+            <version>4.1.8.Final</version>


Any reason why not 4.1.11?

Its release cycle is impressive

4.1.13 😃

leventov · 2017-06-05T19:44:57Z

src/main/java/com/metamx/http/client/HttpClientConfig.java

     * @return encoding name
     */
-    public abstract String getEncodingString();
+    public abstract String getEncodingString(); /*TODO use for Content-Encoding*/


Please file a Github/Jira issue about that

leventov · 2017-06-05T19:57:05Z

src/main/java/com/metamx/http/client/NettyHttpClient.java

-    if (!headers.containsKey(HttpHeaders.Names.HOST)) {
-      httpRequest.headers().add(HttpHeaders.Names.HOST, getHost(url));
+    String uri = urlFile.isEmpty() ? "/" : urlFile;
+    final DefaultFullHttpRequest httpRequest =


suggested if-else instead of ternary

leventov · 2017-06-05T20:07:36Z

src/main/java/com/metamx/http/client/NettyHttpClient.java

-
-                if (httpChunk.isLast()) {
+                response = handler.handleChunk(response, httpChunk);
+                if (response.isFinished() && !retVal.isDone()) {


Why this change? Before this block was executed only if the chunk is not last

Even last chunk has content now thus it should be handled

leventov · 2017-06-05T20:08:01Z

src/main/java/com/metamx/http/client/NettyHttpClient.java

-                  if (response.isFinished() && !retVal.isDone()) {
-                    retVal.set((Final) response.getObj());
-                  }
+                  ctx.close();


Why added closing context?

leventov · 2017-06-05T20:19:00Z

src/main/java/com/metamx/http/client/response/InputStreamResponseHandler.java

+    ByteBuf content = response instanceof HttpContent ? ((HttpContent) response).content() : Unpooled.EMPTY_BUFFER;
    AppendableByteArrayInputStream in = new AppendableByteArrayInputStream();
-    in.add(getContentBytes(response.getContent()));
+    in.add(content.array());


Is there a guarantee that the array exists? ByteBuf has hasArray() method. Also arrayOffset() should be considered.

leventov · 2017-06-05T20:19:07Z

src/main/java/com/metamx/http/client/response/InputStreamResponseHandler.java

  )
  {
-    clientResponse.getObj().add(getContentBytes(chunk.getContent()));
+    clientResponse.getObj().add(chunk.content().array());


leventov · 2017-06-05T20:21:01Z

src/main/java/com/metamx/http/client/response/SequenceInputStreamResponseHandler.java

-    final ChannelBuffer channelBuffer = chunk.getContent();
-    final int bytes = channelBuffer.readableBytes();
-    if (bytes > 0) {
+    if (chunk.content().hasArray()) {


hasArray() means not that there are no bytes, it means that the ByteBuf is backed by off-heap memory.

leventov · 2017-06-05T20:21:16Z

src/main/java/com/metamx/http/client/response/SequenceInputStreamResponseHandler.java

        throw Throwables.propagate(e);
      }
-      byteCount.addAndGet(bytes);
+      byteCount.addAndGet(chunk.content().array().length);


readableBytes()?

leventov · 2017-06-05T20:24:23Z

src/main/java/com/metamx/http/client/response/ToStringResponseHandler.java

  public ClientResponse<StringBuilder> handleResponse(HttpResponse response)
  {
-    return ClientResponse.unfinished(new StringBuilder(response.getContent().toString(charset)));
+    ByteBuf content = response instanceof HttpContent ? ((HttpContent) response).content() : Unpooled.EMPTY_BUFFER;


Suggested if-else and provide "" empty string directly instead of converting empty buffer to empty string

logarithm

Request changes just in order to not forget to test this PR with netty 4.0.42.Final in runtime to make sure it will work with spark 2.1

drcrallen · 2017-06-23T16:42:35Z

@logarithm / @esevastyanov is it possible to organize the POM such that the default profile uses 4.1.x, and we have the ability to specify a profile for 4.0.x so we can test?

drcrallen · 2017-06-23T16:48:13Z

Or we can just raise the issue with spark again and get the upstream on 4.1

logarithm · 2017-06-26T10:10:27Z

To rise issue with spark looks like the best solution, but it might take time.

fenik17 · 2017-07-11T05:40:14Z

src/test/java/com/metamx/http/client/FriendlyServersTest.java

                String header;
                while (!(header = in.readLine()).equals("")) {
-                  if (header.equals("Accept-Encoding: identity")) {
+                  if (header.toLowerCase().equals("Accept-Encoding: identity".toLowerCase())) {


String.equalsIgnoreCase()?

Eugene Sevastianov added 12 commits February 17, 2017 19:45

Netty test server instead of jetty one.

5c42d4f

BACKEND-670: Netty 4.1 migration

dc7b7c2

BACKEND-670: Netty 4.1 migration

ef12bff

BACKEND-670: Netty 4.1 migration

0086be4

BACKEND-670: Netty 4.1 migration

53ea2ae

BACKEND-670: Netty 4.1 migration

2725847

BACKEND-670: Netty 4.1 migration

844f3d1

BACKEND-670: Netty 4.1 migration

1d041b8

BACKEND-670: Netty 4.1 migration

cda1d1b

BACKEND-670: Netty 4.1 migration

7ffc5aa

Merge remote-tracking branch 'origin/master' into netty_instead_of_jetty

058a471

# Conflicts: # pom.xml

BACKEND-670: Netty 4.1 migration

735f0b3

esevastyanov requested a review from leventov June 4, 2017 14:43

leventov suggested changes Jun 5, 2017

View reviewed changes

leventov mentioned this pull request Jun 9, 2017

Druid - Spark interoperation is problematic due to Netty dependency mismatch apache/druid#4390

Closed

logarithm suggested changes Jun 9, 2017

View reviewed changes

BACKEND-670: Netty 4.1 migration. Fixes

e5a8145

fenik17 reviewed Jul 11, 2017

View reviewed changes

Conversation

esevastyanov commented Jun 4, 2017

Uh oh!

drcrallen commented Jun 5, 2017

Uh oh!

drcrallen commented Jun 5, 2017

Uh oh!

logarithm commented Jun 5, 2017

Uh oh!

drcrallen commented Jun 5, 2017

Uh oh!

logarithm commented Jun 5, 2017

Uh oh!

drcrallen commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logarithm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Jun 23, 2017

Uh oh!

drcrallen commented Jun 23, 2017

Uh oh!

logarithm commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

drcrallen commented Jun 5, 2017 •

edited

Loading

logarithm left a comment •

edited

Loading