[SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency #43164
Conversation
common/network-common/pom.xml
Outdated
There was a problem hiding this comment.
Does the network-yarn module have a requirement to use these native codes?
There was a problem hiding this comment.
I don't believe it needs a direct dependency here if you look at the code changes to resource-managers/yarn in #42685 - we do add tests to the yarn code that do pass.
We do change ShuffleTransportContext which is in network-common, and that's what is used by yarn here. network-yarn depends on network-shuffle which depends on network-common so I think that's how the dependency gets pulled in.
There was a problem hiding this comment.
These need to be test scope right?
There was a problem hiding this comment.
@srowen the bouncycastle and network-common-helper dependencies are only needed in the test scope, but the netty-tcnative-boringssl-static dependencies need to be available in the production build. We need this dependency in order to provide SSL support.
pom.xml
Outdated
There was a problem hiding this comment.
What's the license of this code? if it's the same as netty, I think we're all good. Otherwise need to check it and possibly update LICENSE and licenses/
There was a problem hiding this comment.
@srowen netty-tcnative-boringssl-static seems to be apache2 licensed: https://github.com/netty/netty-tcnative/blob/main/LICENSE.txt
which is the same as netty as far as I can tell: https://github.com/netty/netty/blob/4.1/LICENSE.txt
hasnain-db
changed the title
|
tests are flaky, attempting a rebase |
hasnain-db
force-pushed
the
spark-tls-deps
branch
2 times, most recently
from
ce77fb7 to
cc80198
Compare
hasnain-db
force-pushed
the
spark-tls-deps
branch
from
cc80198 to
06a1d8d
Compare
|
@mridulm @JoshRosen this should be good to review now (had to rerun tests a bunch of times due to flakiness) |
|
Merged to master. |
|
@hasnain-db Report a issue. After this pr, the following error occurs when executing the And it could be built successfully before this pr. My testing environment is
Could you please look into this issue? Thanks ~ @hasnain-db also cc @wangyum |
|
I'm able to reproduce this, will take a look, thanks! might be a few days, I hope that is OK - happy to revert otherwise |
|
@LuciferYang I filed https://issues.apache.org/jira/browse/SPARK-45464 to track this. I have a fix locally (just pushed a commit), should submit the PR tonight once CI signal comes back. I'll tag you on the PR. cc @wangyum |
### What changes were proposed in this pull request? Exclude an apache license in the packaged jar to fix the build for network-yarn. Also shade the native libraries so they can be used. ### Why are the changes needed? Making a distribution build using the following command currently fails after #43164: ``` ./dev/make-distribution.sh --tgz -Phive -Phive-thriftserver -Pyarn ``` Failures look like ``` [ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.1.0:run (unpack) on project spark-network-yarn_2.13: An Ant BuildException has occured: Error while expanding /Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar [ERROR] java.nio.file.FileSystemException: /Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/META-INF/license/LICENSE.aix-netbsd.txt: Not a directory [ERROR] around Ant part ...<unzip src="/Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar" dest="/Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/" />... 5:198 in /Users/hasnain.lakhani/spark/common/network-yarn/target/antrun/build-main.xml [ERROR] -> [Help 1] org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.1.0:run (unpack) on project spark-network-yarn_2.13: An Ant BuildException has occured: Error while expanding /Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar java.nio.file.FileSystemException: /Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/META-INF/license/LICENSE.aix-netbsd.txt: Not a directory around Ant part ...<unzip src="/Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar" dest="/Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/" />... 5:198 in /Users/hasnain.lakhani/spark/common/network-yarn/target/antrun/build-main.xml ``` After much trial and error (I added a debugging section for the next person who runs into this) I realized the problem is that inside the jar, the META-inf folder looks like this: ``` META-INF/LICENSE META-INF/LICENSE.txt META-INF/license/ META-INF/license/LICENSE.aix-netbsd.txt META-INF/license/LICENSE.boringssl.txt META-INF/license/LICENSE.mvn-wrapper.txt META-INF/license/LICENSE.tomcat-native.txt ``` Since the filesystem on macosx is case insensitive, this causes issues. Unzipping this fails, as we unzip the file first, then do not create the directory, and fail to unzip the files after that. This PR proposes to fix it by removing the `LICENSE` file, as suggested, by adding it to the exclusion list, so the folder gets copied. The contents are the same as `LICENSE.txt` (which is also just the apache2 license) so this should be fine. We also shade and copy over the new native libraries similar to the existing ones. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? CI I reran ``` ./dev/make-distribution.sh --tgz -Phive -Phive-thriftserver -Pyarn ``` and it succeeded ### Was this patch authored or co-authored using generative AI tooling? No ### Appendix: debugging I was able to debug the contents of the jar using `jar tf $jar` and print out the files using `unzip -q -c $jar $path`. To make iterations go faster I went to `/Users/hasnain.lakhani/spark/common/network-yarn/target/antrun` and edited the ant file by hand then ran `ant -f build-main.xml` Closes #43289 from hasnain-db/tls-fix-build. Authored-by: Hasnain Lakhani <hasnain.lakhani@databricks.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
4 participants
What changes were proposed in this pull request?
This PR adds a new dependency on this library so that we can use it for SSL functionality.
We also include the network-common test helper library in a few places, which will be needed for reusing some test configuration files in the future.
Why are the changes needed?
These dependency changes are needed so we can use this library to provide SSL functionality, and for test helpers.
Does this PR introduce any user-facing change?
No
How was this patch tested?
CI. This is used in unit tests and functionality as part of #42685, from which this is being split out
Was this patch authored or co-authored using generative AI tooling?
No