You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "turboFei (via GitHub)" <gi...@apache.org> on 2023/05/17 14:42:51 UTC
[GitHub] [spark] turboFei opened a new pull request, #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
turboFei opened a new pull request, #41201:
URL: https://github.com/apache/spark/pull/41201
<!--
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'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### 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.
-->
Adding working directory into classpath on the driver in K8S cluster mode.
### 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.
-->
so that the spark.files, spark.jars placed in the working directory can be accessible by the classloader.
For example, the `hive-site.xml` uploaded by `spark.files`.
### 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'.
-->
yes, users do not need to add the working directory into spark classpath manually.
### 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.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
UT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #41201:
URL: https://github.com/apache/spark/pull/41201#issuecomment-1558478053
gentle ping @dongjoon-hyun @holdenk @pralabhkumar @pan3793 for the latest change, thanks a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] advancedxy commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1211709330
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
Review Comment:
> Sorry, but why this is prepended? For this specific part, I'm strong negative because this could have a side-effect like `spark.executor.userClassPathFirst=true`. As we know, we don't recommend `userClassPathFirst` at all.
Hi @dongjoon-hyun, just to be clear, are you against for putting the `PWD` into executor's class path or for the `PWD` being the first in the class path?
In my opinion, to align with spark on yarn's behavior, `PWD` should be put in both driver and executor's class path. But I'm ok for it to be last or anywhere in the class path.
By the way, this pr put `PWD(.)` in the first of class path for driver, if you have concern about `PWD` being first, the driver may have the same issue 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1199937113
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
how about now? @advancedxy
I checked the code, for driver, if just leverage `entrypoint.sh`, it is difficult to keep the behavior as mentioned above.
So I just leverage the `entrypoint.sh` for executor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41201:
URL: https://github.com/apache/spark/pull/41201#issuecomment-1581603207
The `sql - other tests` failure is a known flaky one which is irrelevant to this one. Let me merge this 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] advancedxy commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1200028494
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1196873001
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -425,7 +428,7 @@ private[spark] class SparkSubmit extends Logging {
case (localResources, resolvedUri) =>
val source = new File(localResources.getPath)
val dest = new File(
- ".",
+ workingDirectory,
if (resolvedUri.getFragment != null) resolvedUri.getFragment else source.getName)
Review Comment:
Since `.` is participated in, can we call `File#getCanonicalFile` before assigning `source` and `dest` here to improve the following log format?
```log
Files ... from /tmp/spark-1a68de84-0fb2-4cf6-8f7a-302c24509935/xyz-plugin-spark3-2.12-20230516-131757.jar.jar to /opt/spark/work-dir/./xyz-plugin-spark3-2.12-20230516-131757.jar
```
then the logging statement can be simplified to
```patch
-logInfo(
- s"Files $resolvedUri " +
- s"from ${source.getAbsolutePath} to ${dest.getAbsolutePath}")
+logInfo(s"Files $resolvedUri from $source to $dest")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41201:
URL: https://github.com/apache/spark/pull/41201#issuecomment-1569562222
cc @advancedxy once more because of his https://github.com/apache/spark/pull/41201#discussion_r1200028494 . Now, the last commit reverted `entrypoint.sh` change .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1212011161
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
Review Comment:
Ya. The second one. I'm only worrying about the `prepending`. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] advancedxy commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1212578282
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
Review Comment:
then, @turboFei would you mind to do some search to make sure which place the PWD should be put in the executor’s class path? sorry for the inconvenience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pralabhkumar commented on pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "pralabhkumar (via GitHub)" <gi...@apache.org>.
pralabhkumar commented on PR #41201:
URL: https://github.com/apache/spark/pull/41201#issuecomment-1552336382
LGTM .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197147035
##########
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala:
##########
@@ -1618,6 +1618,24 @@ class SparkSubmitSuite
conf.get(k) should be (v)
}
}
+
+ test("SPARK-43540: Add working directory into classpath on the driver in K8S cluster mode") {
+ val clArgs = Seq(
+ "--deploy-mode", "client",
+ "--proxy-user", "test.user",
+ "--master", "k8s://host:port",
+ "--executor-memory", "5g",
+ "--class", "org.SomeClass",
+ "--driver-memory", "4g",
+ "--conf", "spark.kubernetes.namespace=spark",
+ "--conf", "spark.kubernetes.driver.container.image=bar",
+ "--conf", "spark.kubernetes.submitInDriver=true",
+ "/home/thejar.jar",
+ "arg1")
Review Comment:
thanks for the reminder, minimized the test case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197526307
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
here we have checked the k8s cluster mode
```
if (isKubernetesClusterModeDriver)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1200067293
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun closed pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
URL: https://github.com/apache/spark/pull/41201
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197146855
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -425,7 +428,7 @@ private[spark] class SparkSubmit extends Logging {
case (localResources, resolvedUri) =>
val source = new File(localResources.getPath)
val dest = new File(
- ".",
+ workingDirectory,
if (resolvedUri.getFragment != null) resolvedUri.getFragment else source.getName)
Review Comment:
refactored logging format, 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197261326
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -425,7 +428,7 @@ private[spark] class SparkSubmit extends Logging {
case (localResources, resolvedUri) =>
val source = new File(localResources.getPath)
val dest = new File(
- ".",
+ workingDirectory,
if (resolvedUri.getFragment != null) resolvedUri.getFragment else source.getName)
Review Comment:
addressed, 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1199937113
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
how about now? @advancedxy
I checked the code, for driver, if just leverage the `entrypoint.sh`, it is difficult to keep the behavior as mentioned above.
So I just leverage the `entrypoint.sh` for executor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1196873001
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -425,7 +428,7 @@ private[spark] class SparkSubmit extends Logging {
case (localResources, resolvedUri) =>
val source = new File(localResources.getPath)
val dest = new File(
- ".",
+ workingDirectory,
if (resolvedUri.getFragment != null) resolvedUri.getFragment else source.getName)
Review Comment:
Since `.` is participated in, can we call `File#getCanonicalFile` before assigning `source` and `dest` here to avoid the following message?
```log
Files ... from /tmp/spark-1a68de84-0fb2-4cf6-8f7a-302c24509935/xyz-plugin-spark3-2.12-20230516-131757.jar.jar to /opt/spark/work-dir/./xyz-plugin-spark3-2.12-20230516-131757.jar
```
And the logging statement can be simplified to
```patch
-logInfo(
- s"Files $resolvedUri " +
- s"from ${source.getAbsolutePath} to ${dest.getAbsolutePath}")
+logInfo(s"Files $resolvedUri from $source to $dest")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #41201:
URL: https://github.com/apache/spark/pull/41201#issuecomment-1581801675
thanks all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1218855113
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
Review Comment:
and for executor classpath, I think that we can place the working directory at the tail of 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1196873001
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -425,7 +428,7 @@ private[spark] class SparkSubmit extends Logging {
case (localResources, resolvedUri) =>
val source = new File(localResources.getPath)
val dest = new File(
- ".",
+ workingDirectory,
if (resolvedUri.getFragment != null) resolvedUri.getFragment else source.getName)
Review Comment:
Since `.` is participated in, can we call `File#getCanonicalFile` before assigning `source` and `dest` here to improve the following log format?
```log
Files ... from /tmp/spark-1a68de84-0fb2-4cf6-8f7a-302c24509935/xyz-plugin-spark3-2.12-20230516-131757.jar.jar to /opt/spark/work-dir/./xyz-plugin-spark3-2.12-20230516-131757.jar
```
And the logging statement can be simplified to
```patch
-logInfo(
- s"Files $resolvedUri " +
- s"from ${source.getAbsolutePath} to ${dest.getAbsolutePath}")
+logInfo(s"Files $resolvedUri from $source to $dest")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197607669
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
will check it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1196696629
##########
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala:
##########
@@ -1618,6 +1618,24 @@ class SparkSubmitSuite
conf.get(k) should be (v)
}
}
+
+ test("SPARK-43540: Add working directory into classpath on the driver in K8S cluster mode") {
+ val clArgs = Seq(
+ "--deploy-mode", "client",
+ "--proxy-user", "test.user",
+ "--master", "k8s://host:port",
+ "--executor-memory", "5g",
+ "--class", "org.SomeClass",
+ "--driver-memory", "4g",
+ "--conf", "spark.kubernetes.namespace=spark",
+ "--conf", "spark.kubernetes.driver.container.image=bar",
+ "--conf", "spark.kubernetes.submitInDriver=true",
+ "/home/thejar.jar",
+ "arg1")
Review Comment:
Thank you for making a PR. BTW, do we need 11 lines to assert `classpath`? Do you think you can minimize this test case like the existing test cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] advancedxy commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197525305
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
It might affect other modes. A better place would be the entrypoint.sh for resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh to include `work-dir` into 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197526307
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
here we have checked the k8s cluster mode
```
if (isKubernetesClusterModeDriver) {
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver/executor in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1211004144
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
Review Comment:
thanks for the correction，will check and address it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver/executor in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1211059622
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1218854527
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
Review Comment:
sorry for the late reply, I checked the code.
For driver:
<img width="972" alt="image" src="https://github.com/apache/spark/assets/6757692/b3372777-724d-4952-8755-f0db6fbe7329">
I think for driver, now the working directory is just placed before user jars, likes
- --jars
- localPrimaryResource
It should be 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] turboFei commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1199937113
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
how about now? @advancedxy
I checked the code, for driver, if just leverage `entrypoint.sh`, it is difficult to put CWD before localized SPARK_CONF and HADOOP_CONF.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] advancedxy commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1197555100
##########
core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala:
##########
@@ -414,6 +414,9 @@ private[spark] class SparkSubmit extends Logging {
// directory too.
// SPARK-33782 : This downloads all the files , jars , archiveFiles and pyfiles to current
// working directory
+ // SPARK-43540: add current working directory into classpath
+ val workingDirectory = "."
+ childClasspath += workingDirectory
Review Comment:
This doesn't add current working dir to executor's class path right?
Just checked with yarn's behavior, yarn add `CWD` to both driver and executor. And it puts `CWD` before localized SPARK_CONF and HADOOP_CONF.
See https://github.com/apache/spark/blob/014685c41e4741f83570d8a2a6a253e48967919a/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L1442
To get the similar behavior, I believe it would be easier to leverage the `entrypoint.sh` here when running on K8S.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41201:
URL: https://github.com/apache/spark/pull/41201#issuecomment-1551615074
cc @pralabhkumar and @holdenk from #37417
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] holdenk commented on pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on PR #41201:
URL: https://github.com/apache/spark/pull/41201#issuecomment-1551827663
+1 looks reasonable module the existing suggestions (clean up the logging + tighten the test). Thanks for making this 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #41201: [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver/executor in K8S cluster mode
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41201:
URL: https://github.com/apache/spark/pull/41201#discussion_r1210882681
##########
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh:
##########
@@ -75,6 +75,9 @@ elif ! [ -z ${SPARK_HOME+x} ]; then
SPARK_CLASSPATH="$SPARK_HOME/conf:$SPARK_CLASSPATH";
fi
+# SPARK-43540: add current working directory into executor classpath
+SPARK_CLASSPATH="$PWD:$SPARK_CLASSPATH"
Review Comment:
Sorry, but why this is prepended? For this specific part, I'm strong negative because this could have a side-effect like `spark.executor.userClassPathFirst=true`. As we know, we don't recommend `userClassPathFirst` at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org