You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by trystanleftwich <gi...@git.apache.org> on 2015/03/03 23:39:18 UTC
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
GitHub user trystanleftwich opened a pull request:
https://github.com/apache/spark/pull/4880
[SPARK-6144]When in cluster mode using ADD JAR with a hdfs:// sourced ja...
...r will fail
While in cluster mode if you use ADD JAR with a HDFS sourced jar it will fail trying to source that jar on the worker nodes with the following error:
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/AtScaleInc/spark branch-1.3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/4880.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4880
----
commit 5931cc933f5b33be986089e34494070e93f04733
Author: trystanleftwich <tr...@atscale.com>
Date: 2015-03-03T22:37:24Z
[SPARK-6144]When in cluster mode using ADD JAR with a hdfs:// sourced jar will fail
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4880#discussion_r25738862
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -632,8 +633,9 @@ private[spark] object Utils extends Logging {
fs: FileSystem,
conf: SparkConf,
hadoopConf: Configuration,
- fileOverwrite: Boolean): Unit = {
- if (!targetDir.mkdir()) {
+ fileOverwrite: Boolean,
+ filename: String = ""): Unit = {
--- End diff --
+1 to adding some kind of type safety here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77062488
@pwendell adding to your radar.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4880#discussion_r25740726
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -643,7 +645,12 @@ private[spark] object Utils extends Logging {
fileOverwrite)
} else {
val in = fs.open(innerPath)
- val targetFile = new File(targetDir, innerPath.getName)
+ val targetFile: File =
+ if (!filename.isEmpty) {
+ new File(targetDir, filename)
+ } else {
+ new File(targetDir, innerPath.getName)
+ }
--- End diff --
Is this correct? If `path` refers to a directory with multiple files in it, then this will fetch all of those files using the same name, overwriting all but the last one fetched. IIUC we need to differentiate between `path` being a directory from it being a file in the beginning of this method:
```
// L641, before the listStatus logic
if (fs.isFile(path)) {
val targetFile = new File(targetDir, filename.getOrElse(path.getName))
val in = fs.open(path)
downloadFile(path.toString, in, targetFile, fileOverwrite)
} else {
... // do the listStatus thing we've been doing before
}
```
where `filename` should be set if and only if the path refers to a file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77073312
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28246/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77072245
Ah, I didn't realize `addFile` also supports directories for Hadoop file systems. Then this does seem to a correctness problem.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77060524
LGTM aside from minor style issue. I also think this should really go into 1.3...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77068164
@trystanleftwich thanks for fixing this. I believe given the current way we call `fetchHcfsFile` your existing patch is sufficient in fixing the problem. However, at the risk of being pedantic, I believe it is technically not correct if the path refers to a directory for the reason I described above. This is only a concern with fetching directories from `fetchHcfsFile`, which is something I don't think we support anyway.
In other words I think this patch in its current state is fine to merge, but I'd be interested to hear what others think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77056435
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77071494
I tried this patch locally and while it works for `addFile(String)`, it seems to not work for `addFile(String, boolean)` (i.e. the directory version). Here's the error I got:
Exception in thread "Driver" org.apache.spark.SparkException: File /dataroot/local/yarn/nm/usercache/systest/appcache/application_1425400850634_0015/userFiles-ed754418-8d53-4e86-a324-738b70fab5cd/spark-files.23921
exists and does not match contents of hdfs://vanzin-st1-1.vpc.cloudera.com:8020/tmp/spark-files.23921/core-site.xml
at org.apache.spark.util.Utils$.copyFile(Utils.scala:519)
at org.apache.spark.util.Utils$.org$apache$spark$util$Utils$$downloadFile(Utils.scala:471)
at org.apache.spark.util.Utils$$anonfun$fetchHcfsFile$1.apply(Utils.scala:654)
at org.apache.spark.util.Utils$$anonfun$fetchHcfsFile$1.apply(Utils.scala:641)
Let me take a look to see if I figure out what's missing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77073304
[Test build #28246 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28246/consoleFull) for PR 4880 at commit [`5931cc9`](https://github.com/apache/spark/commit/5931cc933f5b33be986089e34494070e93f04733).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/4880#discussion_r25737673
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -632,8 +633,9 @@ private[spark] object Utils extends Logging {
fs: FileSystem,
conf: SparkConf,
hadoopConf: Configuration,
- fileOverwrite: Boolean): Unit = {
- if (!targetDir.mkdir()) {
+ fileOverwrite: Boolean,
+ filename: String = ""): Unit = {
--- End diff --
I'd use `Option[String] = None`. The in L648 you can do `val targetFile = new File(targetDir, filename.getOrElse(innerPath.getName)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77056801
[Test build #28246 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28246/consoleFull) for PR 4880 at commit [`5931cc9`](https://github.com/apache/spark/commit/5931cc933f5b33be986089e34494070e93f04733).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4880#discussion_r25743237
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -643,7 +645,12 @@ private[spark] object Utils extends Logging {
fileOverwrite)
} else {
val in = fs.open(innerPath)
- val targetFile = new File(targetDir, innerPath.getName)
+ val targetFile: File =
+ if (!filename.isEmpty) {
+ new File(targetDir, filename)
+ } else {
+ new File(targetDir, innerPath.getName)
+ }
--- End diff --
> If the path passed to it is a directory, it will recursively call itself without setting filename
That's not actually true. If you call `listStatus` on a directory it will list the directory's contents and not include the directory itself. So if the directory contains multiple files they will all go into the else case in L646 and be renamed to the same thing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77054200
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by trystanleftwich <gi...@git.apache.org>.
Github user trystanleftwich commented on the pull request:
https://github.com/apache/spark/pull/4880#issuecomment-77079677
I fat fingered and accidentally closed this ticket, And for some reason its not picking up that the branch has changes in it. I reopened here:
https://github.com/apache/spark/pull/4881
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/4880#discussion_r25743304
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -643,7 +645,12 @@ private[spark] object Utils extends Logging {
fileOverwrite)
} else {
val in = fs.open(innerPath)
- val targetFile = new File(targetDir, innerPath.getName)
+ val targetFile: File =
+ if (!filename.isEmpty) {
+ new File(targetDir, filename)
+ } else {
+ new File(targetDir, innerPath.getName)
+ }
--- End diff --
Yes, the problem here is that now `targetDir` is the parent directory of where `path` should be, and the children are being written directly to that parent path. It needs some code to create this directory corresponding to `path` before downloading the children.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by trystanleftwich <gi...@git.apache.org>.
Github user trystanleftwich closed the pull request at:
https://github.com/apache/spark/pull/4880
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6144]When in cluster mode using ADD JAR...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/4880#discussion_r25741212
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -643,7 +645,12 @@ private[spark] object Utils extends Logging {
fileOverwrite)
} else {
val in = fs.open(innerPath)
- val targetFile = new File(targetDir, innerPath.getName)
+ val targetFile: File =
+ if (!filename.isEmpty) {
+ new File(targetDir, filename)
+ } else {
+ new File(targetDir, innerPath.getName)
+ }
--- End diff --
I think it's a little weird but it works. Only the very first call to `fetchHcfsFile` defines `filename`. If the `path` passed to it is a directory, it will recursively call itself without setting `filename`. If it's a file, it will write the file using the given `filename`. So even if it could be clearer, the code as is should work.
But I'm ok with making it clearer, exactly to avoid this kind of discussion. :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org