Skip to content

Conversation

@witgo
Copy link
Contributor

@witgo witgo commented Nov 9, 2016

What changes were proposed in this pull request?

One of the important changes for 4.0.42.Final is "Support any FileRegion implementation when using epoll transport netty/netty#5825".
In 4.0.42.Final, MessageWithHeader can work properly when spark.[shuffle|rpc].io.mode is set to epoll

How was this patch tested?

Existing tests

@witgo witgo changed the title [WIP]Upgrade netty to 4.0.42.Final [WIP][SPARK-18375][BUILD][CORE]Upgrade netty to 4.0.42.Final Nov 9, 2016
@SparkQA
Copy link

SparkQA commented Nov 9, 2016

Test build #68408 has finished for PR 15830 at commit e73c1d9.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@witgo witgo changed the title [WIP][SPARK-18375][BUILD][CORE]Upgrade netty to 4.0.42.Final [WIP][SPARK-18375][SPARK-18383][BUILD][CORE]Upgrade netty to 4.0.42.Final Nov 9, 2016
@witgo witgo changed the title [WIP][SPARK-18375][SPARK-18383][BUILD][CORE]Upgrade netty to 4.0.42.Final [SPARK-18375][SPARK-18383][BUILD][CORE]Upgrade netty to 4.0.42.Final Nov 9, 2016
Copy link
Member

Choose a reason for hiding this comment

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

Seems OK. Do you need to check for a null message though, like above? In fact, the condition above for BindException could be e.getMessage != null || isBindCollision(e.getCause)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the netty code, the return of NativeIoException.getMessage is not null.
The only way to create a NativeIoException instance is to call Errors.newIOException

Copy link
Member

Choose a reason for hiding this comment

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

OK. It wouldn't hurt to be defensive rather than depend on this IMHO. If it changes then suddenly this becomes a mysterious NPE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will modify the code.

@witgo witgo force-pushed the SPARK-18375_netty-4.0.42 branch from e73c1d9 to c4546dc Compare November 10, 2016 13:35
@SparkQA
Copy link

SparkQA commented Nov 10, 2016

Test build #68470 has finished for PR 15830 at commit c4546dc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Nov 12, 2016

Merged to master/2.1

@asfgit asfgit closed this in bc41d99 Nov 12, 2016
asfgit pushed a commit that referenced this pull request Nov 12, 2016
## What changes were proposed in this pull request?

One of the important changes for 4.0.42.Final is "Support any FileRegion implementation when using epoll transport netty/netty#5825".
In 4.0.42.Final, `MessageWithHeader` can work properly when `spark.[shuffle|rpc].io.mode` is set to epoll

## How was this patch tested?

Existing tests

Author: Guoqiang Li <witgo@qq.com>

Closes #15830 from witgo/SPARK-18375_netty-4.0.42.

(cherry picked from commit bc41d99)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@witgo
Copy link
Contributor Author

witgo commented Nov 13, 2016

@srowen thank you

@witgo witgo deleted the SPARK-18375_netty-4.0.42 branch November 14, 2016 01:29
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
## What changes were proposed in this pull request?

One of the important changes for 4.0.42.Final is "Support any FileRegion implementation when using epoll transport netty/netty#5825".
In 4.0.42.Final, `MessageWithHeader` can work properly when `spark.[shuffle|rpc].io.mode` is set to epoll

## How was this patch tested?

Existing tests

Author: Guoqiang Li <witgo@qq.com>

Closes apache#15830 from witgo/SPARK-18375_netty-4.0.42.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants