You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/02/19 11:17:10 UTC
[GitHub] [spark] shardulm94 opened a new pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
shardulm94 opened a new pull request #31591:
URL: https://github.com/apache/spark/pull/31591
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
Ship the `spark.jars.ivySettings` file to the driver when using `cluster` deploy mode so that `addJar` is able to find it in order to resolve dependencies.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
SPARK-33084 introduced support for Ivy paths in `sc.addJar` or Spark SQL `ADD JAR`. If we use a custom ivySettings file using `spark.jars.ivySettings`, it is loaded at https://github.com/apache/spark/blob/b26e7b510bbaee63c4095ab47e75ff2a70e377d7/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L1280. However, this file is only accessible on the client machine. In cluster mode, this file is not available on the driver and so `addJar` fails to find it.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Added unit tests to a) verify that spark-submit marks the `ivySettings` file to be distributed in cluster mode b) a YARN cluster mode application is able to find the `ivySettings` file.
----------------------------------------------------------------
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.
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 change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r580965988
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
Adding it into `__spark__conf__` sounds okay to me ... but I would prefer to have second opinions from Yarn experts such as @tgravescs, @mridulm or @jerryshao.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819138187
--
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-823726696
Thanks @tgravescs and @xkrogen for the reviews on 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.
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] github-actions[bot] commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819840970
**[Test build #592944647](https://github.com/shardulm94/spark/actions/runs/592944647)** for PR 31591 at commit [`fd3ddb2`](https://github.com/shardulm94/spark/commit/fd3ddb2bd60fd34e605479eb07c2d7021f3c44e0).
--
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819718340
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137352/
--
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.
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] tgravescs commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-823511438
merged to master, thanks @shardulm94
--
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786367087
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40064/
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819894653
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137370/
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-800641671
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136128/
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819177710
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137312/
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819894653
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137370/
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819613618
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41931/
--
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.
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 change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r580934692
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
I know you might have to copy `ivysettings.xml` to the current working directory when Spark submit runs but I think it might work for your usecase.
----------------------------------------------------------------
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.
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] mridulm commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-818288578
+1 to @tgravescs's suggestion.
--
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-782351781
@xkrogen
> (2) I know we discussed the possibility of using an approach where the settings file is in the Spark conf zip, did you try this approach and discover any issues / complexity?
I did try and there are some tradeoffs. I was planning to implement this similar to how we ship `log4j.properties` as part of Spark conf. https://github.com/apache/spark/blob/484a83e73e8b9420ffd42b4eb95dcdf4cf66ebf0/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L762 Then we can use `classloader.getResource` to read the ivy file on the driver classpath. However, this approach for shipping the file would be specific to YARN, and won't work with other resource managers with cluster mode unless we make similar change in all resource managers. There is also possibility that `classloader.getResource` may get resources from other jars/zip (not spark_conf.zip) depending on classpath ordering. One alternative can be to search for the unzipped file in spark_conf i.e `__spark_conf__/ivysettings.xml` but again that is specific to YARN's spark conf naming.
I am very new to Spark, so there might be obvious ways of resolving the above concerns that I am missing. Please let me know, I would be happy to try them out.
> (1) There will be a conflict if the user specifies any file to --files with the same name as their spark.jars.ivySettings. This seems like an odd case, so I am not worried about it.
Yes, that is possible, however even in the alternative approaches I thought of (e.g. the one above), there was always a possibility of this happening.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786367087
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40064/
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786333251
**[Test build #135484 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135484/testReport)** for PR 31591 at commit [`3f49d07`](https://github.com/apache/spark/commit/3f49d07ed57ce6e98785c6e3a36b3763562e1548).
----------------------------------------------------------------
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-823509316
Hey @tgravescs, it's been a few days since the approval. Are we waiting for more inputs before merging 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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613430732
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
Review comment:
Filed https://issues.apache.org/jira/browse/SPARK-35074
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819718340
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137352/
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-805599726
**[Test build #136444 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136444/testReport)** for PR 31591 at commit [`a4d76d7`](https://github.com/apache/spark/commit/a4d76d7cb074a1553f2da3a394a26d5ad64ab60e).
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819819092
--
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r598805746
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -368,6 +369,19 @@ class YarnClusterSuite extends BaseYarnClusterSuite {
)
checkResult(finalState, result, "true")
}
+
+ test("SPARK-34472: ivySettings file should be localized on driver in cluster mode") {
+
+ val emptyIvySettings = File.createTempFile("ivy", ".xml")
+ FileUtils.write(emptyIvySettings, "<ivysettings />", StandardCharsets.UTF_8)
+
Review comment:
Just saw this comment, sounds fine to me.
--
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.
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] SparkQA removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819784290
**[Test build #137370 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137370/testReport)** for PR 31591 at commit [`fd3ddb2`](https://github.com/apache/spark/commit/fd3ddb2bd60fd34e605479eb07c2d7021f3c44e0).
--
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.
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] SparkQA removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819118410
**[Test build #137312 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137312/testReport)** for PR 31591 at commit [`d99cc86`](https://github.com/apache/spark/commit/d99cc86fee9a23bfdcb5fd7135f7f8f68e8b4c32).
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581294595
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,47 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 1) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ // Delete the original ivySettings file, so we ensure that the YARN localized file
+ // is used by the addJar call
+ // In a real cluster mode, the original settings file at the absolute path won't be present
+ // on the driver
+ new File(settingsFile).delete()
+
+ val caught = intercept[Exception] {
Review comment:
We can, but only slightly. Changed it to `RuntimeException`.
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613533241
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -368,6 +368,25 @@ class YarnClusterSuite extends BaseYarnClusterSuite {
)
checkResult(finalState, result, "true")
}
+
+ def createEmptyIvySettingsFile: File = {
+ val emptyIvySettings = File.createTempFile("ivy", ".xml")
+ Files.write("<ivysettings />", emptyIvySettings, StandardCharsets.UTF_8)
+ emptyIvySettings
+ }
+
+ test("SPARK-34472: non-local ivySettings file should be localized on driver in cluster mode") {
Review comment:
Thanks for the suggestion! Caught an issue with handling of `file:///`. Added tests for `local` and `hdfs` schemes.
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-805718630
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41028/
--
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613539257
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -518,6 +518,32 @@ private[spark] class Client(
require(localizedPath != null, "Keytab file already distributed.")
}
+ // If we passed in a ivySettings file, make sure we copy the file to the distributed cache
+ // in cluster mode so that the driver can access it
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ val ivySettingsLocalizedPath: Option[String] = ivySettings match {
+ case Some(ivySettingsPath) if isClusterMode =>
+ val uri = new URI(ivySettingsPath)
+ Option(uri.getScheme).getOrElse("file") match {
+ case "file" =>
+ val ivySettingsFile = new File(ivySettingsPath)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(), s"Ivy settings file $ivySettingsFile is not a" +
+ "normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not
Review comment:
SGTM thanks!
--
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581299862
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,47 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
Review comment:
+1
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819610335
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41931/
--
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819614917
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41931/
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-782033367
Can one of the admins verify this patch?
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786340905
**[Test build #135484 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135484/testReport)** for PR 31591 at commit [`3f49d07`](https://github.com/apache/spark/commit/3f49d07ed57ce6e98785c6e3a36b3763562e1548).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r579322484
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,47 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 1) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ // Delete the original ivySettings file, so we ensure that the YARN localized file
+ // is used by the addJar call
+ // In a real cluster mode, the original settings file at the absolute path won't be present
+ // on the driver
+ new File(settingsFile).delete()
+
+ val caught = intercept[Exception] {
Review comment:
Is there any more specific exception type we can catch here?
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,47 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 1) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ // Delete the original ivySettings file, so we ensure that the YARN localized file
+ // is used by the addJar call
+ // In a real cluster mode, the original settings file at the absolute path won't be present
+ // on the driver
+ new File(settingsFile).delete()
+
+ val caught = intercept[Exception] {
+ sc.addJar("ivy://org.fake-project.test:test:1.0.0")
+ }
+ if (caught.getMessage.contains("unresolved dependency: org.fake-project.test#test")) {
+ // "unresolved dependency" is expected as the dependency does not exist
+ // but exception like "Ivy settings file <file> does not exist should result in failure
Review comment:
Minor nit: missing closing quote on this line
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,47 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
Review comment:
There's a lot of duplicated code between all of these test apps for argument parsing / validation and writing out a `result` file. I think we should consider doing a little refactoring to allow them to inherit from a trait which handles some of this shared logic, or to leverage common utility methods.
But I have no complaints if you want to maintain the status quo in this patch instead of trying to tackle that as well. Maybe a good follow-on PR.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819177710
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137312/
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613537991
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -518,6 +518,32 @@ private[spark] class Client(
require(localizedPath != null, "Keytab file already distributed.")
}
+ // If we passed in a ivySettings file, make sure we copy the file to the distributed cache
+ // in cluster mode so that the driver can access it
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ val ivySettingsLocalizedPath: Option[String] = ivySettings match {
+ case Some(ivySettingsPath) if isClusterMode =>
+ val uri = new URI(ivySettingsPath)
+ Option(uri.getScheme).getOrElse("file") match {
+ case "file" =>
+ val ivySettingsFile = new File(ivySettingsPath)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(), s"Ivy settings file $ivySettingsFile is not a" +
+ "normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not
Review comment:
The assertions of file existence are slightly different in both places. In YarnClient we would want to make this assertion only for the `file` scheme before we process it, however in `SparkSubmit` we would make to make the assertion for all schemes after the appropriate download steps. E.g. for `local` scheme too we would want to validate that the final file that `loadIvySettings` will process does exist.
Once we added handling for `local` and `hdfs` schemes, the functions in both classes will differ even more. So I think it is okay to have duplication here.
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581246230
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
I tried this out, and it looks like it can be handled pretty cleanly this way targeting just YARN. https://github.com/shardulm94/spark/pull/1/commits/12709f0def5a20a0448e602d3ac8216396c8dea3
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581294115
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,47 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 1) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ // Delete the original ivySettings file, so we ensure that the YARN localized file
+ // is used by the addJar call
+ // In a real cluster mode, the original settings file at the absolute path won't be present
+ // on the driver
+ new File(settingsFile).delete()
+
+ val caught = intercept[Exception] {
+ sc.addJar("ivy://org.fake-project.test:test:1.0.0")
+ }
+ if (caught.getMessage.contains("unresolved dependency: org.fake-project.test#test")) {
+ // "unresolved dependency" is expected as the dependency does not exist
+ // but exception like "Ivy settings file <file> does not exist should result in failure
Review comment:
Fixed
----------------------------------------------------------------
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.
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] tgravescs commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-817958670
sorry I somehow missed the response. I would be fine with this PR doing a subset of scope as long as we file a jira to add the others and document it appropriately and make sure nothing weird happens if the other schemes are tried.
--
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.
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] xkrogen commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-817951152
Interesting point @shardulm94. My take is that we should definitely support the full range of URIs (remote, `file`, `local`) eventually for the sake of consistency, so we shouldn't make any changes which would hinder this in the future. But an incremental step forward which just brings support for `file` to start makes sense to me, as long as it is extensible for future `local`/remote support.
cc also @mridulm in case you have any new input
--
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786142046
@xkrogen @HyukjinKwon This is ready for a second round of reviews
cc: @mridulm @jerryshao @tgravescs since this deals with YARN
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581298730
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
+ // When running driver in cluster mode, the settingsFile is localized and so needs to be
+ // accessed using just the file name and not the full file path
+ val localizedFile = new File(file.getName)
+ val resolvedFile = if (file.exists()) {
Review comment:
I updated the approach taken by this PR. The implementation is much cleaner IMO, albeit it only handles YARN for now. I am not sure if other resource managers would require a similar feature.
----------------------------------------------------------------
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.
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] xkrogen commented on pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-784411877
> We could enhance the path to support custom schemes like classpath://, but since the code also needs to work inside SparkSubmit where the classpath is still being constructed, this would not work for both SparkSubmit and the driver together.
There are mechanisms to adjust the classpath that the client sees, but the impetus would be on the user/administrator to configure this properly, so it does push up some extra work. That being said, I think your recent proposal about using `__spark_conf__` looks cleaner.
> Also not sure if we should worry about backward compatibility too much here. Being able to use addJar in Spark context is a fairly new feature and hasn't been released yet, its set for 3.2.0.
Totally agreed that for reading `ivysettings.xml` from the driver, it doesn't need backwards-compatibility. But for client-side cases it should be backwards-compatible.
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581277755
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
+ // When running driver in cluster mode, the settingsFile is localized and so needs to be
+ // accessed using just the file name and not the full file path
+ val localizedFile = new File(file.getName)
+ val resolvedFile = if (file.exists()) {
+ Some(file)
+ } else if (localizedFile.exists()) {
Review comment:
Yeah, agreed. I have elaborated on the difficulty to implement this cleanly in https://github.com/apache/spark/pull/31591#issuecomment-783969152
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r580946284
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
Yeah, as you pointed out it only works if I have the copy in the current working directory. However we cannot which directory our users launch spark-submit from.
What do you think of this? In Yarn Client, we can add the ivySettings file to `spark.yarn.dist.files` or `__spark__conf__.zip` and then we can modify the property `spark.jars.ivySettings` to change it to `./ivysettings.xml` or `__spark__conf__/ivysettings.xml`.
----------------------------------------------------------------
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.
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] xkrogen commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819788065
Last diff 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.
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] SparkQA removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786333251
**[Test build #135484 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135484/testReport)** for PR 31591 at commit [`3f49d07`](https://github.com/apache/spark/commit/3f49d07ed57ce6e98785c6e3a36b3763562e1548).
----------------------------------------------------------------
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.
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] shardulm94 edited a comment on pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 edited a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-782351781
@xkrogen
> I know we discussed the possibility of using an approach where the settings file is in the Spark conf zip, did you try this approach and discover any issues / complexity?
I did try and there are some tradeoffs. I was planning to implement this similar to how we ship `log4j.properties` as part of Spark conf. https://github.com/apache/spark/blob/484a83e73e8b9420ffd42b4eb95dcdf4cf66ebf0/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L762 Then we can use `classloader.getResource` to read the ivy file on the driver classpath. However, this approach for shipping the file would be specific to YARN, and won't work with other resource managers with cluster mode unless we make similar change in all resource managers. There is also possibility that `classloader.getResource` may get resources from other jars/zip (not spark_conf.zip) depending on classpath ordering. One alternative can be to search for the unzipped file in spark_conf i.e `__spark_conf__/ivysettings.xml` but again that is specific to YARN's spark conf naming.
I am very new to Spark, so there might be obvious ways of resolving the above concerns that I am missing. Please let me know, I would be happy to try them out.
> (1) There will be a conflict if the user specifies any file to --files with the same name as their spark.jars.ivySettings. This seems like an odd case, so I am not worried about it.
Yes, that is possible, however even in the alternative approaches I thought of (e.g. the one above), there was always a possibility of this happening.
----------------------------------------------------------------
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.
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 change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r580930365
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
@shardulm94, does it work if you set `spark.jars.ivySettings` to `./ivysettings.xml` and pass `ivysettings.xml` to `spark.yarn.files`?
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r600249173
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
Review comment:
I have updated the PR using `distribute()` to localize the ivySettings file instead of putting the file in Spark conf zip. I also had to slightly modify the ivySettings loading code in `SparkSubmit.scala` since it did not support local URIs in the first place. I also added tests for local and non-local URIs for ivySettings.
--
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-783969152
I thought about this a bit more. I think the main concern here is where/how to find the ivySettings file. I believe this becomes tricky because the same code and properties are currently being used to load the ivySettings file in SparkSubmit (where it makes more sense to read the file directly since it will be local and its too early to be put on the classpath) and inside the driver (where it would be better to read this off the classpath, the user provided local path can be invalid).
We could set an internal config like `spark.yarn.xxx` but this property will need to be used inside `core` and I don't see any precedent for yarn specific properties being used in such a way.
@xkrogen We could enhance the path to support custom schemes like `classpath://`, but since the code also needs to work inside SparkSubmit where the classpath is still being constructed, this would not work for both SparkSubmit and the driver together. I guess one thing we can do is modify `spark.jars.ivySettings` to convert it from a file entry to a classpath entry before passing it to the driver? Also not sure if we should worry about backward compatibility too much here. Being able to use addJar in Spark context is a fairly new feature and hasn't been released yet, its set for 3.2.0.
Another option is to have separate code for loading ivySettings file in SparkSubmit v/s driver and call the appropriate method, or pass an explicit parameter to the `loadIvySettings` function from the driver which instructs it to only search the classpath.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819165990
**[Test build #137312 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137312/testReport)** for PR 31591 at commit [`d99cc86`](https://github.com/apache/spark/commit/d99cc86fee9a23bfdcb5fd7135f7f8f68e8b4c32).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581297470
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,47 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
Review comment:
Given the amount of discussion around the primary change itself, I think its best to keep this as a followup. I have added a note in SPARK-34472.
----------------------------------------------------------------
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r580396470
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
+ // When running driver in cluster mode, the settingsFile is localized and so needs to be
+ // accessed using just the file name and not the full file path
+ val localizedFile = new File(file.getName)
+ val resolvedFile = if (file.exists()) {
Review comment:
+1 that if we go the `spark.files` route we should probably keep a separate (internal) config to pass the file name info. Similar to `spark.yarn.user.jar`, `spark.yarn.cache.filenames`, etc.
----------------------------------------------------------------
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.
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] tgravescs commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r600502967
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -131,6 +131,18 @@ private[spark] class Client(
require(keytab == null || !Utils.isLocalUri(keytab), "Keytab should reference a local file.")
+ private val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ private val ivySettingsLocalizedFileName: Option[String] = ivySettings match {
+ case Some(ivySettingsPath) if isClusterMode && !Utils.isLocalUri(ivySettingsPath) =>
+ val ivySettingsFile = new File(ivySettingsPath)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
Review comment:
this isn't going to work if the file is in HDFS, the distribute call would handle that.
Can you please test that this works pulling from HDFS all the way through for both cluster and client mode?
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1277,7 +1277,11 @@ private[spark] object SparkSubmitUtils {
settingsFile: String,
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
- val file = new File(settingsFile)
+ val file = if (Utils.isLocalUri(settingsFile)) {
+ new File(new URI(settingsFile).getPath)
+ } else {
+ new File(settingsFile)
Review comment:
this won't work with HDFS files, right? I think we would need to download the file.
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-805737046
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41028/
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819890797
**[Test build #137370 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137370/testReport)** for PR 31591 at commit [`fd3ddb2`](https://github.com/apache/spark/commit/fd3ddb2bd60fd34e605479eb07c2d7021f3c44e0).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613537991
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -518,6 +518,32 @@ private[spark] class Client(
require(localizedPath != null, "Keytab file already distributed.")
}
+ // If we passed in a ivySettings file, make sure we copy the file to the distributed cache
+ // in cluster mode so that the driver can access it
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ val ivySettingsLocalizedPath: Option[String] = ivySettings match {
+ case Some(ivySettingsPath) if isClusterMode =>
+ val uri = new URI(ivySettingsPath)
+ Option(uri.getScheme).getOrElse("file") match {
+ case "file" =>
+ val ivySettingsFile = new File(ivySettingsPath)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(), s"Ivy settings file $ivySettingsFile is not a" +
+ "normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not
Review comment:
The assertions of file existence are slightly different in both places. In YarnClient we would want to make this assertion only for the `file` scheme before we process it, however in `SparkSubmit` we would make to make the assertion for all schemes after the appropriate download steps. E.g. for `local` scheme too we would want to validate that the final file that `loadIvySettings` will process does exist.
Once we add handling for `local` and `hdfs` schemes, the functions in both classes will differ even more. So I think it is okay to have duplication here.
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-805688312
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41028/
--
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-812750599
@tgravescs @xkrogen `spark.jar.ivySettings` today only supports files present on the client. It does not support `local://` or `hdfs://` files. Can the scope of this PR be restricted to making the current file support work in cluster mode?
Trying to add `local://` and `hdfs://` support would be nice, but there doesn't seems to be a requirement yet and it is making things unnecessarily complicated. I think most of the complexity stems from the fact the ivySettings loading code in SparkSubmit is shared between both the Spark client and the Spark driver, the way to access the files in these two contexts seems very different.
Since the restriction for ivySettings file being client local already existed before, I am guessing most users who need a custom ivySettings file have it configured locally and we can address adding support for other schemes later if need arises.
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r580946284
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
Yeah, as you pointed out it only works if I have the copy in the current working directory. However we cannot control which directory our users launch spark-submit from. So ideally we would want something which works without user intervention.
What do you think of this? In Yarn Client, we can add the ivySettings file to `spark.yarn.dist.files` or `__spark__conf__.zip` and then we can modify the property `spark.jars.ivySettings` to change it to `./ivysettings.xml` or `__spark__conf__/ivysettings.xml`.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819819127
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41948/
--
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819138380
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41892/
--
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613342371
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +602,44 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 2) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file] [expected ivy settings prefix]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val expectedIvySettingsPrefix = args(1)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ assert(settingsFile.startsWith(expectedIvySettingsPrefix))
Review comment:
Should we also `assert(settingsFile !== expectedIvySettingsPrefix)` ?
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -368,6 +368,25 @@ class YarnClusterSuite extends BaseYarnClusterSuite {
)
checkResult(finalState, result, "true")
}
+
+ def createEmptyIvySettingsFile: File = {
+ val emptyIvySettings = File.createTempFile("ivy", ".xml")
+ Files.write("<ivysettings />", emptyIvySettings, StandardCharsets.UTF_8)
+ emptyIvySettings
+ }
+
+ test("SPARK-34472: non-local ivySettings file should be localized on driver in cluster mode") {
Review comment:
Can we also test `clientMode = true` and assert that the file name hasn't been modified?
Could also be good to add tests for explicitly using `file:///` URI and for an expected failure when using `local:///` URI?
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -518,6 +518,32 @@ private[spark] class Client(
require(localizedPath != null, "Keytab file already distributed.")
}
+ // If we passed in a ivySettings file, make sure we copy the file to the distributed cache
+ // in cluster mode so that the driver can access it
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ val ivySettingsLocalizedPath: Option[String] = ivySettings match {
+ case Some(ivySettingsPath) if isClusterMode =>
+ val uri = new URI(ivySettingsPath)
+ Option(uri.getScheme).getOrElse("file") match {
+ case "file" =>
+ val ivySettingsFile = new File(ivySettingsPath)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(), s"Ivy settings file $ivySettingsFile is not a" +
+ "normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not
Review comment:
This block of code (and the IllegalArgumentException below) -- convert to URI, check scheme, throw if bad scheme, assert on file existence -- is duplicated between `SparkSubmit` and `Client`. Can we put a method like `getIvySettingsFile` into `SparkSubmit` which is leveraged in both places?
Not too big of a duplication so if it can't be done cleanly no worries from my end.
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786339347
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40064/
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786346295
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135484/
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r581299070
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
I updated the PR with this approach.
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r595538679
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
Review comment:
This would help save some RPCs in an HDFS environment, more context in
https://github.com/apache/spark/pull/31591#pullrequestreview-594367541 See drawback (2)
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819138380
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41892/
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613532867
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +602,44 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 2) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file] [expected ivy settings prefix]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val expectedIvySettingsPrefix = args(1)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ assert(settingsFile.startsWith(expectedIvySettingsPrefix))
Review comment:
Done
--
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.
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] tgravescs commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r598850998
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
Review comment:
I get that its kind of like a conf file but its not, there is no template file in the conf directory for this, it's a user specified config and not automatically picked up, so I would rather keep things consistent and have it treated like all the other configs similar to it.
Also we don't want to support this being in HDFS? I think that comes for free if you just use distribute, I don't see a few rpc calls as being a big deal here especially since it adds functionality. I guess if you allow that you might need more code on the client side to download it. Its a bit unfortunately we are not consistent with these things.
Personally I would prefer us to stay consistent and this act just like other files users can specify. it should allow being in HDFS and it should be distributed via distribute() just like all the other files users can specify. If you disagree please let me know why.
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-800661805
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40710/
----------------------------------------------------------------
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r583760800
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
Review comment:
This would be a little more Scala-idiomatic for unpacking the `Option`:
```
ivySettings match {
case Some(ivySettingsPath) if isClusterMode =>
// ... logic here
case _ => // do nothing
}
```
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
+ Files.copy(ivySettingsFile, confStream)
Review comment:
Can we use `java.nio.file.Files` here instead of Guava's `Files`? I know Guava is already used in this file but I think it's better to leverage the built-in functionality moving forward. You can do a rename import like `import java.nio.file.{Files => NioFiles}` (or update the other references as well if it's a small change?)
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,44 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 1) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ // Make sure that ivySettings conf was set to the localized file
+ assert(settingsFile.startsWith(Client.LOCALIZED_CONF_DIR))
+
+ val caught = intercept[RuntimeException] {
+ sc.addJar("ivy://org.fake-project.test:test:1.0.0")
+ }
+ if (caught.getMessage.contains("unresolved dependency: org.fake-project.test#test")) {
+ // "unresolved dependency" is expected as the dependency does not exist
+ // but exception like "Ivy settings file <file> does not exist should result in failure"
Review comment:
Minor nit: end quote is in the wrong place
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -368,6 +369,19 @@ class YarnClusterSuite extends BaseYarnClusterSuite {
)
checkResult(finalState, result, "true")
}
+
+ test("SPARK-34472: ivySettings file should be localized on driver in cluster mode") {
+
+ val emptyIvySettings = File.createTempFile("ivy", ".xml")
+ FileUtils.write(emptyIvySettings, "<ivysettings />", StandardCharsets.UTF_8)
+
Review comment:
Can we use NIO for these? No need for commons-io now that NIO supports this kind of stuff built-in.
```
val emptyIvySettings = Files.createTempFile(...)
Files.write(emptyIvySettings, Seq(...))
```
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819784290
**[Test build #137370 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137370/testReport)** for PR 31591 at commit [`fd3ddb2`](https://github.com/apache/spark/commit/fd3ddb2bd60fd34e605479eb07c2d7021f3c44e0).
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819819127
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41948/
--
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.
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] tgravescs commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r598815261
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
Review comment:
that's fine.
--
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.
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] shardulm94 edited a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 edited a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-812750599
@tgravescs @xkrogen `spark.jar.ivySettings` today only supports files present on the client. It does not support `local://` or `hdfs://` files. Can the scope of this PR be restricted to making the current file support work in cluster mode?
Trying to add `local://` and `hdfs://` support would be nice, but there doesn't seem to be a requirement yet and it is making things unnecessarily complicated. I think most of the complexity stems from the fact the ivySettings loading code in SparkSubmit is shared between both the Spark client and the Spark driver, the way to access the files in these two contexts seems very different.
Since the restriction for ivySettings file being client local already existed before, I am guessing most users who need a custom ivySettings file have it configured locally and we can address adding support for other schemes later if need arises.
--
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-805632645
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136444/
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819570700
**[Test build #137352 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137352/testReport)** for PR 31591 at commit [`fab9f9e`](https://github.com/apache/spark/commit/fab9f9ee4e78260c1c8e272efc794a142a1682d2).
--
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r600644547
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -131,6 +131,18 @@ private[spark] class Client(
require(keytab == null || !Utils.isLocalUri(keytab), "Keytab should reference a local file.")
+ private val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ private val ivySettingsLocalizedFileName: Option[String] = ivySettings match {
+ case Some(ivySettingsPath) if isClusterMode && !Utils.isLocalUri(ivySettingsPath) =>
+ val ivySettingsFile = new File(ivySettingsPath)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
Review comment:
+1
It would be much nicer if we could rely on `distribute` instead of trying to figure out for ourselves here if it needs to be uploaded, e.g. `distribute` will handle the local URI for us.
I think the reason we have to do this here is because `createConfArchive` doesn't give us a way to pass in the `ivySettingsLocalizedFileName` information. Can we just update `createConfArchive` to accept a parameter like `configOverrides: Map[String, String]` which provides a way to override configs that need to be different for the YARN container environment? Then we can move this within `prepareLocalResources` and rely more on the logic of `distribute`.
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819118410
**[Test build #137312 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137312/testReport)** for PR 31591 at commit [`d99cc86`](https://github.com/apache/spark/commit/d99cc86fee9a23bfdcb5fd7135f7f8f68e8b4c32).
--
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.
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] SparkQA removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819570700
**[Test build #137352 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137352/testReport)** for PR 31591 at commit [`fab9f9e`](https://github.com/apache/spark/commit/fab9f9ee4e78260c1c8e272efc794a142a1682d2).
--
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819716804
**[Test build #137352 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137352/testReport)** for PR 31591 at commit [`fab9f9e`](https://github.com/apache/spark/commit/fab9f9ee4e78260c1c8e272efc794a142a1682d2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819614917
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41931/
--
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819600340
@tgravescs @xkrogen I have updated the PR to restrict scope to the `file://` scheme. A path without scheme is assumed to be `file://`. Other schemes will throw an IllegalArgumentException. Please take a look.
I have also filed SPARK-35072 and SPARK-35073 for future improvements and linked it to SPARK-34472.
--
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r595557365
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,44 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 1) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ // Make sure that ivySettings conf was set to the localized file
+ assert(settingsFile.startsWith(Client.LOCALIZED_CONF_DIR))
+
+ val caught = intercept[RuntimeException] {
+ sc.addJar("ivy://org.fake-project.test:test:1.0.0")
+ }
+ if (caught.getMessage.contains("unresolved dependency: org.fake-project.test#test")) {
+ // "unresolved dependency" is expected as the dependency does not exist
+ // but exception like "Ivy settings file <file> does not exist should result in failure"
Review comment:
Fixed
##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -368,6 +369,19 @@ class YarnClusterSuite extends BaseYarnClusterSuite {
)
checkResult(finalState, result, "true")
}
+
+ test("SPARK-34472: ivySettings file should be localized on driver in cluster mode") {
+
+ val emptyIvySettings = File.createTempFile("ivy", ".xml")
+ FileUtils.write(emptyIvySettings, "<ivysettings />", StandardCharsets.UTF_8)
+
Review comment:
I changed this to use Guava's Files which is used in many places within this file. Can I create a followup PR to replace these with NIO?
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
Review comment:
Done
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
+ Files.copy(ivySettingsFile, confStream)
Review comment:
Done
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-805737046
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41028/
--
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-782033367
----------------------------------------------------------------
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.
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] tgravescs commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r598815261
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
Review comment:
that's fine, please file an 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.
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] xkrogen commented on pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-782437318
Good point about other resource managers. I'm not familiar enough with Kubernetes or Mesos to have any opinion there or understand if the same problem exists. Hopefully someone more knowledgeable can chime in.
One other idea I've been toying with is to treat this similar to `spark.sql.hive.metastore.jars`:
```
val HIVE_METASTORE_JARS = buildStaticConf("spark.sql.hive.metastore.jars")
.doc(s"""
| Location of the jars that should be used to instantiate the HiveMetastoreClient.
| This property can be one of four options:
...
| 3. "path"
| Use Hive jars configured by `spark.sql.hive.metastore.jars.path`
| in comma separated format. Support both local or remote paths.The provided jars
| should be the same version as ${HIVE_METASTORE_VERSION}.
| 4. A classpath in the standard format for both Hive and Hadoop. The provided jars
| should be the same version as ${HIVE_METASTORE_VERSION}.
""".stripMargin)
```
With this config (besides options 1 and 2 which aren't relevant here), you can either supply classpath entries like `dep1.jar:dep2.jar` or, if you use the `path` option you use a file path with no special handling. If you specify an absolute local path, the responsibility is yours to make sure that local path exists on all worker nodes. This also reminds me that an absolute local path which is expected to be present on all nodes _is_ a valid use case, which may be a reason not to go with the current proposal where we intercept the `spark.jars.ivySettings` and always put it into the distributed cache.
Similar to this Hive JARs conf, we could enhance `spark.jars.ivySettings` to accept a URI instead of just a local file path, allowing for either local or remote paths. We can set up some custom scheme like `classpath://ivysettings.xml` to indicate that the URI points to a classpath entry. Users have the flexibility to use some local path deployed to all nodes, point to a single remote path, or put the file onto the classpath using a mechanism like `--jars`, placing it into `SPARK_CONF_DIR`, etc. URIs without a scheme are treated as local paths for backwards-compatibility. WDYT?
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-805632645
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136444/
--
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.
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] tgravescs commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-788079584
so I guess this case is just if the user does sc.addJar("ivy:..") so you wouldn't know at submission time and can't necessarily download the file in client before submission. One issue with yarn clusters and likely k8s cluster is that network might be more restricted on the cluster itself rather than the client where you submit. But I guess either way that would fail.
With k8s if everything isn't in the docker image you would have to configure a separate path to upload things to (like s3). It looks like this change is only for YARN so I'm assuming this doesn't work with k8s either then? Or standalone mode with cluster mode? I'm fine with fixing for just yarn with this but we should document what it works for.
note that spark specifically supports the "local:" prefix to mean local files. This means we shouldn't upload them as they are local to each machine. https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/util/Utils.scala#L103
Is there a reason we don't just call distributed() like we do for key tab file or the app jar for instance? If we are worried about it conflicting with something else in --files then just give it a unique name
----------------------------------------------------------------
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.
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] asfgit closed pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #31591:
URL: https://github.com/apache/spark/pull/31591
--
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.
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] shardulm94 commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-800622075
@tgravescs Sorry for the delay, time flies!
This PR started off try to address this issue in the general cluster case using `--files` to distribute the ivy setting file for all resource managers. However one of the concerns was that we would have to look for the ivy file different based on whether the driver was running locally v/s on the cluster, since the codepath is shared between SparkSubmit and sc.addJar. https://github.com/apache/spark/blob/0d62c5cd023095afd7081355ec2ff1a8558fed7f/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L1290 There is more context at https://github.com/apache/spark/pull/31591#issuecomment-782351781 and https://github.com/apache/spark/pull/31591#issuecomment-782351781
So I decided to make this specific to YARN since I am not familiar with the other resource managers.
----------------------------------------------------------------
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.
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] tgravescs commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r598850998
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
Review comment:
I get that its kind of like a conf file but its not, there is no template file in the conf directory for this, it's a user specified config and not automatically picked up, so I would rather keep things consistent and have it treated like all the other configs similar to it.
Also we don't want to support this being in HDFS? I think that comes for free if you just use distribute, I don't see a few rpc calls as being a big deal here especially since it adds functionality. I guess if you allow that you might need more code on the client side to download it. Its a bit unfortunately we are not consistent with these things.
Personally I would prefer us to stay consistent and this act just like other files users can specify. it should allow being in HDFS and it should be distributed via distribute() just like all the other files users can specify.
--
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.
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] github-actions[bot] commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-819691105
**[Test build #592944647](https://github.com/shardulm94/spark/actions/runs/592944647)** for PR 31591 at commit [`fab9f9e`](https://github.com/shardulm94/spark/commit/fab9f9ee4e78260c1c8e272efc794a142a1682d2).
--
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r598857412
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
Review comment:
> I get that its kind of like a conf file but its not, there is no template file in the conf directory for this, it's a user specified config and not automatically picked up ... Its a bit unfortunately we are not consistent with these things.
The same is (mostly) true for examples like `metrics.properties`, which is an external/non-Spark file, the only difference being that it is automatically picked up vs. being requested by a user. I don't have much opinion on whether or not this is how it should be -- I agree with you that more clear guidelines/consistency in this area would be nice.
I don't have too much opinion between storing in the conf object and leveraging `distribute`, I do agree that we should support remote file systems (e.g. HDFS) so this is a good point. @HyukjinKwon do you have any opinion here since you participated in some of the earlier conversations?
--
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.
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] AmplabJenkins removed a comment on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-800641671
----------------------------------------------------------------
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.
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 change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r579775478
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
+ // When running driver in cluster mode, the settingsFile is localized and so needs to be
+ // accessed using just the file name and not the full file path
+ val localizedFile = new File(file.getName)
+ val resolvedFile = if (file.exists()) {
Review comment:
I think you should rather have a configuration to point out Ivy settings, and set together with `spark.files`.
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r580946284
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
Review comment:
Yeah, as you pointed out it only works if I have the copy in the current working directory. However we cannot control which directory our users launch spark-submit from. So ideally we would want something which works without user intervention.
What do you think of this? In Yarn Client, we can add the ivySettings file to `spark.yarn.dist.files` or `__spark__conf__.zip` and then we can modify the property `spark.jars.ivySettings` to change it to `./ivysettings.xml` or `__spark__conf__/ivysettings.xml` within Yarn Client.
----------------------------------------------------------------
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.
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 #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786321348
ok to test
----------------------------------------------------------------
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.
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] tgravescs commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r584857419
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
Review comment:
any reason not to just use distribute() rather then putting into the conf files?
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
Review comment:
it would be nice to move this into the configuration package.scala and use ConfigBuilder. Even if we just reference it by the .key option in the SparkSubmitArguments file.
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
Review comment:
also check to see if Utils.isLocalUri
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31591:
URL: https://github.com/apache/spark/pull/31591#issuecomment-786350732
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40064/
----------------------------------------------------------------
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.
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] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r595542214
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file, that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" + UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
Review comment:
I think also, semantically, this is more similar to a Spark configuration file than a user-provided file and thus makes sense to have within the conf zip as opposed to living with the user JARs/files. This is similar to the approach used for `log4j.properties` and `metrics.properties` above.
----------------------------------------------------------------
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.
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] shardulm94 commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
shardulm94 commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r595538004
##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
Review comment:
Would it be okay to handle this as a followup immediately after this PR? There are about 7-8 other places where this string is hardcoded and we can also refactor them out into config package.scala
----------------------------------------------------------------
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.
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 change in pull request #31591: [SPARK-34472][CORE] Ship ivySettings file to driver in cluster mode
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r579775368
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -1278,11 +1287,22 @@ private[spark] object SparkSubmitUtils {
remoteRepos: Option[String],
ivyPath: Option[String]): IvySettings = {
val file = new File(settingsFile)
- require(file.exists(), s"Ivy settings file $file does not exist")
- require(file.isFile(), s"Ivy settings file $file is not a normal file")
+ // When running driver in cluster mode, the settingsFile is localized and so needs to be
+ // accessed using just the file name and not the full file path
+ val localizedFile = new File(file.getName)
+ val resolvedFile = if (file.exists()) {
+ Some(file)
+ } else if (localizedFile.exists()) {
Review comment:
Doesn't it pick the file in the current working directory? It wouldn't make sense if you're running it on non-cluster modes.
----------------------------------------------------------------
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.
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