Netty version update 3.9 -> 4.1 #29
Conversation
# Conflicts: # pom.xml
|
Should do a major version update |
|
On a side note, I'm excited to try this out: https://github.com/netty/netty/tree/4.1/codec-http2 |
|
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. |
|
@logarithm do we have a record of what the problem was? |
|
@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. |
pom.xml
Outdated
| <artifactId>netty</artifactId> | ||
| <version>3.10.6.Final</version> | ||
| <artifactId>netty-all</artifactId> | ||
| <version>4.1.8.Final</version> |
There was a problem hiding this comment.
Any reason why not 4.1.11?
There was a problem hiding this comment.
Its release cycle is impressive
| @@ -56,7 +56,7 @@ public String getEncodingString() | |||
| * | |||
| * @return encoding name | |||
| */ | |||
| public abstract String getEncodingString(); | |||
| public abstract String getEncodingString(); /*TODO use for Content-Encoding*/ | |||
There was a problem hiding this comment.
Please file a Github/Jira issue about that
| if (!headers.containsKey(HttpHeaders.Names.HOST)) { | ||
| httpRequest.headers().add(HttpHeaders.Names.HOST, getHost(url)); | ||
| String uri = urlFile.isEmpty() ? "/" : urlFile; | ||
| final DefaultFullHttpRequest httpRequest = |
There was a problem hiding this comment.
suggested if-else instead of ternary
|
|
||
| if (httpChunk.isLast()) { | ||
| response = handler.handleChunk(response, httpChunk); | ||
| if (response.isFinished() && !retVal.isDone()) { |
There was a problem hiding this comment.
Why this change? Before this block was executed only if the chunk is not last
There was a problem hiding this comment.
Even last chunk has content now thus it should be handled
| if (response.isFinished() && !retVal.isDone()) { | ||
| retVal.set((Final) response.getObj()); | ||
| } | ||
| ctx.close(); |
There was a problem hiding this comment.
Why added closing context?
| AppendableByteArrayInputStream in = new AppendableByteArrayInputStream(); | ||
| in.add(getContentBytes(response.getContent())); | ||
| in.add(content.array()); |
There was a problem hiding this comment.
Is there a guarantee that the array exists? ByteBuf has hasArray() method. Also arrayOffset() should be considered.
| ) | ||
| { | ||
| clientResponse.getObj().add(getContentBytes(chunk.getContent())); | ||
| clientResponse.getObj().add(chunk.content().array()); |
| final ChannelBuffer channelBuffer = chunk.getContent(); | ||
| final int bytes = channelBuffer.readableBytes(); | ||
| if (bytes > 0) { | ||
| if (chunk.content().hasArray()) { |
There was a problem hiding this comment.
hasArray() means not that there are no bytes, it means that the ByteBuf is backed by off-heap memory.
| @@ -112,7 +111,7 @@ public InputStream nextElement() | |||
| Thread.currentThread().interrupt(); | |||
| throw Throwables.propagate(e); | |||
| } | |||
| byteCount.addAndGet(bytes); | |||
| byteCount.addAndGet(chunk.content().array().length); | |||
| @@ -35,21 +36,22 @@ public ToStringResponseHandler(Charset charset) | |||
| @Override | |||
| 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; | |||
There was a problem hiding this comment.
Suggested if-else and provide "" empty string directly instead of converting empty buffer to empty string
|
@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? |
|
Or we can just raise the issue with spark again and get the upstream on 4.1 |
|
To rise issue with spark looks like the best solution, but it might take time. |
| @@ -123,7 +124,7 @@ public void run() | |||
| // Read headers | |||
| String header; | |||
| while (!(header = in.readLine()).equals("")) { | |||
| if (header.equals("Accept-Encoding: identity")) { | |||
| if (header.toLowerCase().equals("Accept-Encoding: identity".toLowerCase())) { | |||
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