You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "snmvaughan (via GitHub)" <gi...@apache.org> on 2023/06/20 19:16:25 UTC
[GitHub] [spark] snmvaughan opened a new pull request, #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
snmvaughan opened a new pull request, #41678:
URL: https://github.com/apache/spark/pull/41678
### What changes were proposed in this pull request?
Propagate proxy settings to forked unit test JVMs. System properties that start with `http.` or `https.` are copied into the `jvmOptions` of the forked processes, ensuring that required proxy settings are available during the tests.
### Why are the changes needed?
Running a subset of unit tests fail in enterprise environments that have additional constraints on network access that are handled by using a proxy. Examples of impacted tests:
- HadoopVersionInfoSuite, test name: SPARK-32256: Hadoop VersionInfo should be preloaded.
- HiveClientUserNameSuite(2.0), test name: 2.0: username of HiveClient - no UGI.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
A local CI build was modified to use Spark branches (3.3 and 3.4) that included this change to verify it addressed the test failures. In addition, logging changes were introduced to verify that the system properties were absent when running the tests prior to the change.
--
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] snmvaughan commented on pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1601848295
The only problem I've run into are tests that use the local hostname instead of `localhost`. This was easily addressed by adding the local hostname to the list in `http.nonProxyHosts`
--
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] dongjoon-hyun closed pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
URL: https://github.com/apache/spark/pull/41678
--
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] dongjoon-hyun commented on a diff in pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41678:
URL: https://github.com/apache/spark/pull/41678#discussion_r1235761038
##########
project/SparkBuild.scala:
##########
@@ -342,6 +342,18 @@ object SparkBuild extends PomBuild {
}
},
+ // Copy system properties to forked JVMs so that tests know proxy settings
Review Comment:
Did you try to use `SBT_OPTS` instead of this?
--
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 pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1605828159
Late LGTM
--
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] HyukjinKwon commented on a diff in pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41678:
URL: https://github.com/apache/spark/pull/41678#discussion_r1237845727
##########
project/SparkBuild.scala:
##########
@@ -1580,6 +1580,19 @@ object TestSettings {
"SPARK_TESTING" -> "1",
"JAVA_HOME" -> sys.env.get("JAVA_HOME").getOrElse(sys.props("java.home")),
"SPARK_BEELINE_OPTS" -> "-DmyKey=yourValue"),
+
+ // Copy system properties to forked JVMs so that tests know proxy settings
Review Comment:
No biggie but I would place the JIRA number here, e.g.)
```suggestion
// Copy system properties to forked JVMs so that tests know proxy settings, see SPARK-44110.
```
--
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] HyukjinKwon commented on pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1601848622
Seems fine to me too.
--
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] snmvaughan commented on a diff in pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on code in PR #41678:
URL: https://github.com/apache/spark/pull/41678#discussion_r1237123228
##########
project/SparkBuild.scala:
##########
@@ -342,6 +342,18 @@ object SparkBuild extends PomBuild {
}
},
+ // Copy system properties to forked JVMs so that tests know proxy settings
Review Comment:
Setting SBT_OPTS does not work, resulting in the same issue.
--
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 pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1600606386
> Running a subset of unit tests fail in enterprise environments that have additional constraints on network access that are handled by using a proxy. Examples of impacted tests:
>
> HadoopVersionInfoSuite, test name: [SPARK-32256](https://issues.apache.org/jira/browse/SPARK-32256): Hadoop VersionInfo should be preloaded.
> HiveClientUserNameSuite(2.0), test name: 2.0: username of HiveClient - no UGI.
@snmvaughan Can you provide the test commands and the failure logs? I think the description here is a bit unclear
On the other hand, could you help check if Maven needs to make similar changes?
--
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] snmvaughan commented on a diff in pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on code in PR #41678:
URL: https://github.com/apache/spark/pull/41678#discussion_r1237844369
##########
project/SparkBuild.scala:
##########
@@ -342,6 +342,18 @@ object SparkBuild extends PomBuild {
}
},
+ // Copy system properties to forked JVMs so that tests know proxy settings
+ javaOptions ++= {
Review Comment:
I've moved the update into TestSettings and switched to "Test / javaOptions". I've verified these changes in my local environment.
--
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] dongjoon-hyun commented on a diff in pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41678:
URL: https://github.com/apache/spark/pull/41678#discussion_r1237733354
##########
project/SparkBuild.scala:
##########
@@ -342,6 +342,18 @@ object SparkBuild extends PomBuild {
}
},
+ // Copy system properties to forked JVMs so that tests know proxy settings
+ javaOptions ++= {
Review Comment:
Given the description, it sounds like a runtime error instead of a compile error. Then, `TestSettings` via `(Test / javaOptions)`?
https://github.com/apache/spark/blob/10751dc285c5c639e3343a8abc26857407522822/project/SparkBuild.scala#L1563-L1583
--
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] dongjoon-hyun commented on pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1603473764
The linter test passed. And, the sql module failure is a known flaky one.
Merged to master for Apache Spark 3.5.0
Thank you, @Stelyus and @HyukjinKwon .
--
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] dongjoon-hyun commented on pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1603308392
Thank you for rebasing, @snmvaughan .
--
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] snmvaughan commented on pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1600980512
The command I've been using for testing disables gigahorse and passes values for the following system properties through PROXY_OPTS:
- `http.proxyHost`
- `http.proxyPort`
- `https.proxyHost`
- `https.proxyPort`
- `http.nonProxyHosts`
```
sbt -Dsbt.gigahorse=false ${PROXY_OPTS} -Phadoop-3 -Phive "hive/test" -Dtest.exclude.tags=org.apache.spark.tags.ExtendedHiveTest
```
The end result is that the CI build fails due to a timeout, with a long list of repeated messages like the following.
```
[info] *** Test still running after 1 hour, 41 minutes, 11 seconds: suite name: HiveClientUserNameSuite(2.0), test name: 2.0: username of HiveClient - no UGI.
[info] *** Test still running after 1 hour, 44 minutes, 59 seconds: suite name: HadoopVersionInfoSuite, test name: SPARK-32256: Hadoop VersionInfo should be preloaded.
```
--
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] dongjoon-hyun commented on pull request #41678: [SPARK-44110][BUILD] Propagate proxy settings to forked JVMs
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41678:
URL: https://github.com/apache/spark/pull/41678#issuecomment-1602003296
`Linter` pipeline seems to fail. Could you take a look at that or re-trigger it simply, @snmvaughan ?
--
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