HADOOP-11245. Update NFS gateway to use Netty4 #2832
Merged
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
jojochuang
added
the
work in progress
|
💔 -1 overall
This message was automatically generated. |
jojochuang
force-pushed
the
netty4migration
branch
from
7b60b99 to
d8256fe
Compare
|
I realized this PR breaks YARN because the netty3 dependency is removed from hdfs, and then transitively YARN doesn't have the netty3 dependency. As a result, when YARN NM starts, it couldn't find netty3 required by the MR shuffle handler. I'll post a follow-up to add explicit dependency for the MR shuffle handler so we can proceed with this one first, leaving the YARN side along. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
jojochuang
removed
the
work in progress
|
As of the last commit, the code was deployed on a small cluster, verified to contain no memory leak via -Dio.netty.leakDetectionLevel=paranoid. The performance saw no noticeable change: prior to the change, the throughput is 39.3MB/s; after the change, the throughput is 38.2MB/s (writing a 1GB file) |
Hi @jojochuang , according to https://netty.io/wiki/reference-counted-objects.html , the option is renamed to |
szetszwo
reviewed
Apr 13, 2021
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcResponse.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
isLast should be volatile.
There was a problem hiding this comment.
i can change it to volatile, but the RpcUtil object is created for each channel, it's not reused. I don't think this is needed.
There was a problem hiding this comment.
Is run() only used in unit tests? If not, workerGroup needs to be shutdown when oneShot == false.
There was a problem hiding this comment.
it's actually used by UT only. I'll add @VisibleForTesting to make it clear.
(The only exception is TestOutOfOrderWrite class. It is a test but written in a standalone application style, not in JUnit style. So, still a test)
Comment on lines
81
to
82
There was a problem hiding this comment.
This should be moved to a finally block. Otherwise, it won't be shutdown in case of exceptions.
There was a problem hiding this comment.
If should call close() instead of closeFuture() since closeFuture() just returns the future but won't close the channel.
There was a problem hiding this comment.
https://netty.io/4.1/api/io/netty/channel/ChannelFuture.html
Do not call await() inside ChannelHandler
The event handler methods in ChannelHandler are usually called by an I/O thread. If await() is called by an event handler method, which is called by the I/O thread, the I/O operation it is waiting for might never complete because await() can block the I/O operation it is waiting for, which is a dead lock.
There was a problem hiding this comment.
I'll rewrite this part according to the official doc.
szetszwo
reviewed
Apr 14, 2021
There was a problem hiding this comment.
@jojochuang , just have finished reviewing all the files. The changes look great! Some minor comments inlined.
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/SimpleTcpServer.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/SimpleTcpServer.java
Outdated
Show resolved
Hide resolved
Comment on lines
44
to
+49
There was a problem hiding this comment.
Should the non-final fields (boundPort, server, ch, bossGroup, workerGroup) be volatile? Not sure if they are synchronized in somewhere.
There was a problem hiding this comment.
these are all private members. Only the shutdown() and run() methods touch them, and both methods are called by the same thread. So given the current usage volatile isn't necessary.
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/SimpleUdpServer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The new code uses wrappedBuffer but old code uses copiedBuffer. Is it intended?
There was a problem hiding this comment.
Intentional. As suspected in the TODO above, it seems wrapping instead of copying the buffer does work.
Comment on lines
150
to
154
There was a problem hiding this comment.
Call bind for both channels first and then sync.
ChannelFuture tcpChannelFuture = tcpServer.bind(tcpAddress);
ChannelFuture udpChannelFuture = udpServer.bind(udpAddress);
tcpChannel = tcpChannelFuture.sync().channel();
udpChannel = udpChannelFuture.sync().channel();
There was a problem hiding this comment.
Should it re-throws the exception? Otherwise, the code below referencing tcpChannel/udpChannel may throw NullPointerException.
Comment on lines
+63
to
+64
There was a problem hiding this comment.
Why setting timeout to 1 second? Is this for debugging?
There was a problem hiding this comment.
So this one I am not 100% sure. Without calling super(), the default is no idle timeout (not sure about the default behavior in netty3), TestPortmap#testIdle() will fail. So I have to set a timeout. If I set a long timeout like 60 seconds, TestPortmap#testIdle() will fail too.
hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/portmap/TestPortmap.java
Outdated
Show resolved
Hide resolved
Change-Id: Ifcba28c153b76035542f2f3ab0ba0e50cb8a713d
…loading MR shuffle handler. Change-Id: Iaa0611bd5dcc0928813d81637abe9cec3ae1e113 (cherry picked from commit 570a299e90e1708399d8d24fb5b6d73569351e80)
Change-Id: I368a69748eb405b6c793198ccef42fa0440d6144
Flush after write ensures data gets written out. There could be opportunity for optimization (for example, flush only once after multiple writes) Change-Id: If11d05d3ee45c0752538d926130daacef7c7e865 (cherry picked from commit 46255e527df48dba2f6774c6e8a480cd4d4b7d51)
Change-Id: Ie26b6b94e3d5143f92ae85cbdaa96da68005cc91 (cherry picked from commit 109f870880e2d35d1d496a297c68893aacf83aad)
Change-Id: Icb1e84324bbfab275930090bb65341ac2771fdea
Change-Id: I2eb9444a97709030263d1840f21c7c83c175884f
jojochuang
force-pushed
the
netty4migration
branch
from
17dfc35 to
229ce95
Compare
|
@szetszwo did you get the chance to review the update? Thanks again! |
szetszwo
approved these changes
Apr 28, 2021
|
Thanks Nicholas! |
kiran-maturi
pushed a commit
to kiran-maturi/hadoop
that referenced
this pull request
Nov 24, 2021
Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]>
hotcodemacha
pushed a commit
to hotcodemacha/hadoop
that referenced
this pull request
Oct 10, 2022
Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]>
jojochuang
added a commit
that referenced
this pull request
Oct 10, 2022
Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]> Co-authored-by: Wei-Chiu Chuang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 10, 2022
Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]> Co-authored-by: Wei-Chiu Chuang <[email protected]> (cherry picked from commit 6847ec0)
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
Oct 11, 2022
…4997) Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]> Co-authored-by: Wei-Chiu Chuang <[email protected]> (cherry picked from commit 6847ec0) Conflicts: hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java
jojochuang
added a commit
that referenced
this pull request
Oct 11, 2022
Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]> Co-authored-by: Wei-Chiu Chuang <[email protected]> (cherry picked from commit 6847ec0) Conflicts: hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java Co-authored-by: Ashutosh Gupta <[email protected]>
jojochuang
added a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]> (cherry picked from commit f41a368) Conflicts: hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/WriteCtx.java hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/WriteManager.java hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/TestOutOfOrderWrite.java Change-Id: I88aa9a1cad18e612d9dd4c7213e1f68d8ae762f9 (cherry picked from commit 51c28dec70ce749742d9941e589678ece5b0dcc9)
DremioQA
pushed a commit
to dremio/hadoop
that referenced
this pull request
Jun 14, 2023
…ache#2832) (apache#4997) Reviewed-by: Tsz-Wo Nicholas Sze <[email protected]> Co-authored-by: Wei-Chiu Chuang <[email protected]> (cherry picked from commit 6847ec0) Change-Id: I0723da91f430a37d4c021e1283a3f729e68be2c7
prabhjyotsingh
added a commit
to acceldata-io/hadoop
that referenced
this pull request
Feb 15, 2024
prabhjyotsingh
added a commit
to acceldata-io/hadoop
that referenced
this pull request
Feb 15, 2024
prabhjyotsingh
added a commit
to acceldata-io/hadoop
that referenced
this pull request
Mar 26, 2024
netty - part1 (cherry picked from commit d94759b)
DremioQA
pushed a commit
to dremio/hadoop
that referenced
this pull request
Apr 5, 2024
This list captures the current state of non-upstream changes in our branch that are not in the public repo. ---Changes cherry-picked to branch-3.3.6-dremio from branch-3.3.2-dremio--- The below changes were on branch-3.3.2-dremio and needed to be brought to branch-3.3.6-dremio to prevent regressing scenarios these changes addressed. HADOOP-18928: S3AFileSystem URL encodes twice where Path has trailing / (proposed) DX-69726: Bumping okie from 1.6.0 to 3.4.0 (CVE-2023-3635) DX-69726: Bumping okie from 1.6.0 to 3.4.0 (CVE-2023-3635) DX-66470: Allow for custom shared key signer for ABFS DX-66673: Backport HADOOP-18602. Remove netty3 dependency DX-66673: Backport MAPREDUCE-7434. Fix ShuffleHandler tests. Contributed by Tamas Domok DX-66673: Backport MAPREDUCE-7431. ShuffleHandler refactor and fix after Netty4 upgrade. (apache#5311) DX-66673: Backport HADOOP-15327. Upgrade MR ShuffleHandler to use Netty4 apache#3259. Contributed by Szilard Nemeth. DX-66673: Backport HADOOP-17115. Replace Guava Sets usage by Hadoop's own Sets in hadoop-common and hadoop-tools (apache#2985) HADOOP-18676. jettison dependency override in hadoop-common lib DX-52816: Downgrade azure-data-lake-store-sdk to 2.3.3 to support dremio version. DX-52701: Remove node based module by Naveen Kumar DX-32012: Adding BatchList Iterator for ListFiles by “ajmeera.nagaraju” DX-18552: Make file status check optional in S3AFileSystem create() Add flag to skip native tests by Laurent Goujon DX-21904: Support S3 requester-pays headers by Brandon Huang DX-21471: Fix checking of use of OAuth credentials with AzureNativeFileSystem DX-19314: make new kms format configurable DX-17058 Add FileSystem to META-INF/services DX-17317 Fix incorrect parameter passed into AzureADAuthenticator-getTokenUsingClientCreds by TiffanyLam DX-17276 Azure AD support for StorageV1 by James Duong DX-17276 Add Azure AD support in Dremio's hadoop-azure library for Storage V1 support unwraps BindException in HttpServer2 ---Changes picked up by moving to 3.3.6--- The below changes were changes on branch-3.3.2-dremio that did not need to come to branch-3.3.6-dremio as the public 3.3.6 branch contained the fixes already. DX-67500: Backport HADOOP-18136. Verify FileUtils.unTar() handling of missing .tar files. DX-66673: Backport HADOOP-18079. Upgrade Netty to 4.1.77. (apache#3977) DX-66673: Backport HADOOP-11245. Update NFS gateway to use Netty4 (apache#2832) (apache#4997) DX-64051: Bump jettison from 1.1 to 1.5.4 in hadoop/branch-3.3.2-dremio DX-64051: Bump jettison from 1.1 to 1.5.4 in hadoop/branch-3.3.2-dremio DX-63800 Bump commons-net from 3.6 to 3.9.0 to address CVE-2021-37533 DX-27168: removing org.codehaus.jackson Change-Id: I6cdb968e33826105caff96e1c3d2c6313a550689
shubhluck
added a commit
to acceldata-io/hadoop
that referenced
this pull request
May 2, 2024
* ODP-1095 CVE Fix jettison upgrade (cherry picked from commit c4e0492) * gson upgraded to 2.9.0 (cherry picked from commit 60d566d) * ODP-1098: Upgrade jackson from 2.10.5 to 2.13.2.2 (cherry picked from commit 5eb67fd) * ODP-1099 | Upgrade jetty version to 9.4.43.v20210629 (cherry picked from commit 1ac7946) * ODP-1104 | snappy-java to 1.1.10.4, snappy-java to 1.1.10.4 (cherry picked from commit 56016fb) * ODP-1103: HADOOP-11245. Update NFS gateway to use Netty4 (apache#2832) netty - part1 (cherry picked from commit d94759b) * ODP-1103: HADOOP-15327. Upgrade MR ShuffleHandler to use Netty4 apache#3259. Contributed by Szilard Nemeth. netty - part2 (cherry picked from commit ee0f478) # Conflicts: # hadoop-project/pom.xml * ODP-1104 | update guava.version to 32.0.1-jre (cherry picked from commit d45a329) * YARN-9081. Update jackson from 1.9.13 to 2.x in hadoop-yarn-services-core. HADOOP-15983. Use jersey-json that is built to use jackson2 (apache#3988) (cherry picked from commit 1545e6c) * ODP-1119-snakeyaml dependency: upgrade to v2.0 (cherry picked from commit 4d1c080) * ODP-1098: add javax.ws.rs-api - 2.1.1 dependency YARN-11558 - Fix dependency convergence error on hbase2 profile (cherry picked from commit 5a9a65d) * ODP-1104 | mvn dependency fix for snappy-java in hadoop-yarn-server-timelineservice-hbase-tests/pom.xml (cherry picked from commit a41ad5c) * TAG change 3.2.3.3.2.2.0-1095 (cherry picked from commit 8567f12) * TAG change2 3.2.3.3.2.2.0-1095 (cherry picked from commit 61e898d) * TAG change3 3.2.3.3.2.3.0-1095 (cherry picked from commit 58d1b91) * TAG change4 3.2.3.3.2.2.0-2 (cherry picked from commit a813e1d) * ODP-1095: set hadoop version as 3.2.3.3.2.2.0-1095 (cherry picked from commit d12de04) * HADOOP-18950. Use shaded avro jar (cherry picked from commit 509824a) * ODP-1103|netty4 upgrade to 4.1.94 (cherry picked from commit 627108d) * jettison dependency exclusion from hadoop-common (cherry picked from commit 0da4db2) * zookeeper release corrected to 3.2.2.0-1095 (cherry picked from commit 4a8bebf) * excluded jackson-core-asl from hadoop-yarn-server-timelineservice-hbase-tests * distribution management addition * ODP-1103: remove netty jar from hadoop-yarn-server-timelineservice-hbase-tests * HADOOP-18512. Upgrade woodstox-core to 5.4.0 for security fix * fixed typo * HADOOP-17033. Update commons-codec from 1.11 to 1.14 * Fixed maven pom across all pom * Removing not required file --------- Co-authored-by: manishsinghmowall <[email protected]> Co-authored-by: kravii <[email protected]> Co-authored-by: Prabhjyot Singh <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
JIRA: HADOOP-11245
This is a draft. It passed unit tests but need functional tests to ensure things like memory leak, performance is good. Looking for additional pairs of eyes to help with the code review.