HADOOP-15327. Upgrade MR ShuffleHandler to use Netty4 #3259
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
shuzirra
reviewed
Aug 4, 2021
.../hadoop-mapreduce-client/hadoop-mapreduce-client-shuffle/src/test/resources/log4j.properties
Outdated
Show resolved
Hide resolved
...reduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/LoggingHttpResponseEncoder.java
Outdated
Show resolved
Hide resolved
...reduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/LoggingHttpResponseEncoder.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
shuzirra
reviewed
Aug 5, 2021
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
szilard-nemeth
force-pushed
the
HADOOP-15327-snemeth
branch
from
148287b to
7922573
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Is there any update on this PR? @szilard-nemeth We are also waiting for the change of using netty4 to replace netty3 due to its vulnerability issues. |
|
Hi @jasonwzs, |
|
Thanks @szilard-nemeth . Will there be any issue if we backport this change to 3.2.x? |
szilard-nemeth
force-pushed
the
HADOOP-15327-snemeth
branch
from
7922573 to
6ba9c41
Compare
Hopefully not but we need to take care of this. |
|
Hi @jasonwzs, |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @szilard-nemeth , I referred PR provided in this jira for netty upgrade for fixing Netty CVE vulnerability and did some fixes for maven shading issues. I am happy to contribute the patch to open source. Can i attach patch in this PR or open a new PR. Let me know your thoughts. Thanks |
|
Hi @ayushpal24, |
|
💔 -1 overall
This message was automatically generated. |
ayushpal24
force-pushed
the
HADOOP-15327-snemeth
branch
from
d244fcf to
31287c0
Compare
|
Hi @ayushpal24, |
|
Hi @szilard-nemeth , some imports got auto reformatted in previous commit , so i amended the changes in that commit. I have verified the changes are not significant .I will check for Maven shading issue after the current build . |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @szilard-nemeth ,this is my first patch and i am not able to find any detail report to understand the reason of failures, any suggestions? |
Hi, |
|
💔 -1 overall
This message was automatically generated. |
szilard-nemeth
force-pushed
the
HADOOP-15327-snemeth
branch
from
31287c0 to
7f43fd6
Compare
|
💔 -1 overall
This message was automatically generated. |
9uapaw
reviewed
Apr 5, 2022
There was a problem hiding this comment.
Thanks for the PR and the huge effort on it @szilard-nemeth. I had a few suggestions/questions in the first round of my review.
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...doop-mapreduce-client-shuffle/src/test/java/org/apache/hadoop/mapred/TestShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
Comment on lines
198
to
199
There was a problem hiding this comment.
This is actually threadsafe according to the documentation. However, GlobalEventExecutor instance is a non-scalable option, perhaps a custom executor will be needed here.
There was a problem hiding this comment.
Fixed, please check again.
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
...t/hadoop-mapreduce-client-shuffle/src/main/java/org/apache/hadoop/mapred/ShuffleHandler.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
… 5 thread instance of DefaultEventExecutorGroup
szilard-nemeth
force-pushed
the
HADOOP-15327-snemeth
branch
from
f6c3739 to
6f36b38
Compare
brumi1024
approved these changes
Nov 10, 2022
|
💔 -1 overall
This message was automatically generated. |
hotcodemacha
approved these changes
Nov 11, 2022
|
Merged to trunk. Thanks @szilard-nemeth for the patch and @shuzirra @9uapaw @K0K0V0K @ashutoshcipher for the review! |
jojochuang
reviewed
Nov 18, 2022
| LOG.debug("Fetcher " + id + " going to fetch from " + host + " for: " | ||
| + maps); | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("Fetcher " + id + " going to fetch from " + host + " for: " + maps); |
There was a problem hiding this comment.
slf4j logger messages can be rewritten using parameterized logging format. But let's not worry about that now. This PR is already too big.
| import static io.netty.handler.codec.http.HttpResponseStatus.OK; | ||
| import static io.netty.handler.codec.http.HttpResponseStatus.UNAUTHORIZED; | ||
| import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1; | ||
| import static org.apache.hadoop.mapred.ShuffleHandler.NettyChannelHelper.*; |
There was a problem hiding this comment.
let's not use wildcard import
| private ServerBootstrap bootstrap; | ||
| private Channel ch; | ||
| private final ChannelGroup accepted = | ||
| new DefaultChannelGroup(new DefaultEventExecutorGroup(5).next()); |
There was a problem hiding this comment.
So, if I understand it correct from the context, the size of the channel group should be maxShuffleConnections, which is unlimited by default (configurable via mapreduce.shuffle.max.connections)
| if ((maxShuffleConnections > 0) && (accepted.size() >= maxShuffleConnections)) { | ||
| NettyChannelHelper.channelActive(ctx.channel()); | ||
| int numConnections = activeConnections.incrementAndGet(); | ||
| if ((maxShuffleConnections > 0) && (numConnections > maxShuffleConnections)) { |
There was a problem hiding this comment.
Perhaps these channel bookeeping is no longer needed given that channel size is limited.
| } else if (cause instanceof IOException) { | ||
| if (cause instanceof ClosedChannelException) { | ||
| LOG.debug("Ignoring closed channel error", cause); | ||
| LOG.debug("Ignoring closed channel error, channel id: " + ch.id(), cause); |
There was a problem hiding this comment.
Use parameterized logging, otherwise wrap it in a LOG.isDebugEnabled() check.
|
Closing this PR as it's already merged into trunk. Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute