You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hasnain-db (via GitHub)" <gi...@apache.org> on 2023/09/28 05:40:36 UTC
[GitHub] [spark] hasnain-db opened a new pull request, #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
hasnain-db opened a new pull request, #43164:
URL: https://github.com/apache/spark/pull/43164
### 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 https://github.com/apache/spark/pull/42685, from which this is being split out
### Was this patch authored or co-authored using generative AI tooling?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43164:
URL: https://github.com/apache/spark/pull/43164#issuecomment-1745907953
Merged to master.
Thanks for adding this @hasnain-db !
Thanks for the review @srowen, @LuciferYang :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43164:
URL: https://github.com/apache/spark/pull/43164#discussion_r1340211898
##########
common/network-common/pom.xml:
##########
@@ -61,6 +61,26 @@
<artifactId>netty-transport-native-kqueue</artifactId>
<classifier>osx-x86_64</classifier>
</dependency>
+ <dependency>
Review Comment:
Does the `network-yarn` module have a requirement to use these native codes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hasnain-db commented on a diff in pull request #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43164:
URL: https://github.com/apache/spark/pull/43164#discussion_r1340506652
##########
pom.xml:
##########
@@ -928,6 +929,30 @@
<version>${netty.version}</version>
<classifier>osx-x86_64</classifier>
</dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #43164:
URL: https://github.com/apache/spark/pull/43164#discussion_r1340408481
##########
common/network-common/pom.xml:
##########
@@ -61,6 +61,26 @@
<artifactId>netty-transport-native-kqueue</artifactId>
<classifier>osx-x86_64</classifier>
</dependency>
+ <dependency>
Review Comment:
These need to be test scope right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43164:
URL: https://github.com/apache/spark/pull/43164#issuecomment-1751949491
@hasnain-db Report a issue.
After this pr, the following error occurs when executing the `./dev/make-distribution.sh --tgz -Phive -Phive-thriftserver -Pyarn` command for packaging:
```
[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/yangjie01/SourceCode/git/spark-mine-13/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar
[ERROR] java.nio.file.FileSystemException: /Users/yangjie01/SourceCode/git/spark-mine-13/common/network-yarn/target/exploded/META-INF/license/LICENSE.aix-netbsd.txt: Not a directory
[ERROR] around Ant part ...<unzip src="/Users/yangjie01/SourceCode/git/spark-mine-13/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar" dest="/Users/yangjie01/SourceCode/git/spark-mine-13/common/network-yarn/target/exploded/" />... @ 5:232 in /Users/yangjie01/SourceCode/git/spark-mine-13/common/network-yarn/target/antrun/build-main.xml
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[ERROR]
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR] mvn <args> -rf :spark-network-yarn_2.13
```
And it could be built successfully before this pr.
My testing environment is
- MacOs: 13.5.2
- Java: openjdk version "17.0.8" 2023-07-18 LTS
- Maven: Apache Maven 3.9.4 (dfbb324ad4a7c8fb0bf182e6d91b0ae20e3d2dd9)
Could you please look into this issue? @hasnain-db
also cc @wangyum
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43164:
URL: https://github.com/apache/spark/pull/43164#issuecomment-1752183229
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hasnain-db commented on a diff in pull request #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43164:
URL: https://github.com/apache/spark/pull/43164#discussion_r1340506652
##########
pom.xml:
##########
@@ -928,6 +929,30 @@
<version>${netty.version}</version>
<classifier>osx-x86_64</classifier>
</dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #43164:
URL: https://github.com/apache/spark/pull/43164#discussion_r1340504595
##########
pom.xml:
##########
@@ -928,6 +929,30 @@
<version>${netty.version}</version>
<classifier>osx-x86_64</classifier>
</dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
Review 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/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43164: [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency
URL: https://github.com/apache/spark/pull/43164
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hasnain-db commented on a diff in pull request #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43164:
URL: https://github.com/apache/spark/pull/43164#discussion_r1340492129
##########
common/network-common/pom.xml:
##########
@@ -61,6 +61,26 @@
<artifactId>netty-transport-native-kqueue</artifactId>
<classifier>osx-x86_64</classifier>
</dependency>
+ <dependency>
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43164:
URL: https://github.com/apache/spark/pull/43164#issuecomment-1752152871
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hasnain-db commented on pull request #43164: [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43164:
URL: https://github.com/apache/spark/pull/43164#issuecomment-1741220260
tests are flaky, attempting a rebase
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45376][CORE] Add netty-tcnative-boringssl-static dependency [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43164:
URL: https://github.com/apache/spark/pull/43164#issuecomment-1743515721
@mridulm @JoshRosen this should be good to review now (had to rerun tests a bunch of times due to flakiness)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hasnain-db commented on a diff in pull request #43164: [SPARK-44937][CORE] Add netty-tcnative-boringssl-static dependency
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43164:
URL: https://github.com/apache/spark/pull/43164#discussion_r1340388063
##########
common/network-common/pom.xml:
##########
@@ -61,6 +61,26 @@
<artifactId>netty-transport-native-kqueue</artifactId>
<classifier>osx-x86_64</classifier>
</dependency>
+ <dependency>
Review Comment:
I don't believe it needs a direct dependency here if you look at the code changes to `resource-managers/yarn` in https://github.com/apache/spark/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org