[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/19885


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r160617532
  
--- Diff: 
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/ClientSuite.scala
 ---
@@ -357,6 +357,41 @@ class ClientSuite extends SparkFunSuite with Matchers {
 sparkConf.get(SECONDARY_JARS) should be (Some(Seq(new 
File(jar2.toURI).getName)))
   }
 
+  private val matching = Seq(
+("files URI match test1", "file:///file1", "file:///file2"),
+("files URI match test2", "file:///c:file1", "file://c:file2"),
+("files URI match test3", "file://host/file1", "file://host/file2"),
+("wasb URI match test", "wasb://bucket1@user", "wasb://bucket1@user/"),
+("hdfs URI match test", "hdfs:/path1", "hdfs:/path1")
+  )
+
+  matching.foreach {
--- End diff --

nit:

```
matching.foreach { t => 
...
```


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r160617569
  
--- Diff: 
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/ClientSuite.scala
 ---
@@ -357,6 +357,41 @@ class ClientSuite extends SparkFunSuite with Matchers {
 sparkConf.get(SECONDARY_JARS) should be (Some(Seq(new 
File(jar2.toURI).getName)))
   }
 
+  private val matching = Seq(
+("files URI match test1", "file:///file1", "file:///file2"),
+("files URI match test2", "file:///c:file1", "file://c:file2"),
+("files URI match test3", "file://host/file1", "file://host/file2"),
+("wasb URI match test", "wasb://bucket1@user", "wasb://bucket1@user/"),
+("hdfs URI match test", "hdfs:/path1", "hdfs:/path1")
+  )
+
+  matching.foreach {
+t =>
+  test(t._1) {
+assert(Client.compareUri(new URI(t._2), new URI(t._3)),
+  s"No match between ${t._2} and ${t._3}")
+  }
+  }
+
+  private val unmatching = Seq(
+("files URI unmatch test1", "file:///file1", "file://host/file2"),
+("files URI unmatch test2", "file://host/file1", "file:///file2"),
+("files URI unmatch test3", "file://host/file1", "file://host2/file2"),
+("wasb URI unmatch test1", "wasb://bucket1@user", 
"wasb://bucket2@user/"),
+("wasb URI unmatch test2", "wasb://bucket1@user", 
"wasb://bucket1@user2/"),
+("s3 URI unmatch test", "s3a://user@pass:bucket1/", 
"s3a://user2@pass2:bucket1/"),
+("hdfs URI unmatch test1", "hdfs://namenode1/path1", 
"hdfs://namenode1:8080/path2"),
+("hdfs URI unmatch test2", "hdfs://namenode1:8020/path1", 
"hdfs://namenode1:8080/path2")
+  )
+
+  unmatching.foreach {
+t =>
--- End diff --

ditto.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[vanzin]

Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r155110589
  
--- Diff: 
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala 
---
@@ -1428,6 +1428,12 @@ private object Client extends Logging {
   return false
 }
 
+val srcAuthority = srcUri.getAuthority()
+val dstAuthority = dstUri.getAuthority()
+if (srcAuthority != null && 
!srcAuthority.equalsIgnoreCase(dstAuthority)) {
--- End diff --

This very method uses `Objects.equal` to solve that problem for hosts below.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r154891674
  
--- Diff: 
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala 
---
@@ -1428,6 +1428,12 @@ private object Client extends Logging {
   return false
 }
 
+val srcAuthority = srcUri.getAuthority()
+val dstAuthority = dstUri.getAuthority()
+if (srcAuthority != null && 
!srcAuthority.equalsIgnoreCase(dstAuthority)) {
--- End diff --

Now, if src is null and dst is not, this fails to detect they're different. 
Please see my previous comment again.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[merlintang]

Github user merlintang commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r154827513
  
--- Diff: 
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala 
---
@@ -1428,6 +1428,12 @@ private object Client extends Logging {
   return false
 }
 
+val srcAuthority = srcUri.getAuthority()
+val detAuthority = dstUri.getAuthority()
+if (srcAuthority != detAuthority || (srcAuthority != null && 
!srcAuthority.equalsIgnoreCase(detAuthority))) {
--- End diff --

thanks all, I would update the PR soon. 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r154822603
  
--- Diff: 
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala 
---
@@ -1428,6 +1428,12 @@ private object Client extends Logging {
   return false
 }
 
+val srcAuthority = srcUri.getAuthority()
+val detAuthority = dstUri.getAuthority()
+if (srcAuthority != detAuthority || (srcAuthority != null && 
!srcAuthority.equalsIgnoreCase(detAuthority))) {
--- End diff --

I guess this line is too long.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r154816425
  
--- Diff: 
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala 
---
@@ -1428,6 +1428,12 @@ private object Client extends Logging {
   return false
 }
 
+val srcAuthority = srcUri.getAuthority()
+val detAuthority = dstUri.getAuthority()
--- End diff --

dst? instead of det?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/19885#discussion_r154819072
  
--- Diff: 
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala 
---
@@ -1428,6 +1428,12 @@ private object Client extends Logging {
   return false
 }
 
+val srcAuthority = srcUri.getAuthority()
+val detAuthority = dstUri.getAuthority()
+if (srcAuthority != detAuthority || (srcAuthority != null && 
!srcAuthority.equalsIgnoreCase(detAuthority))) {
--- End diff --

This compares two strings in a case-sensitive and case-insensitive way; I 
presume it's supposed to be case-insensitive but this will cause authority 
"foo" and "Foo" to return false though they're the same string ignoring case. 
Did this mean a null-safe comparison like ...

```
if ((srcAuthority == null && dstAuthority != null) || 
(srcAuthority != null && !srcAuthority.equalsIgnoreCase(dstAuthority)) {
```


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19885: [SPARK-22587] Spark job fails if fs.defaultFS and...

[merlintang]

GitHub user merlintang opened a pull request:

https://github.com/apache/spark/pull/19885

[SPARK-22587] Spark job fails if fs.defaultFS and application jar are d…

…ifferent url

## What changes were proposed in this pull request?

Two filesystems comparing does not consider the authority of URI. 
Therefore, we have to add the authority to compare two filesystem, and  two 
filesystem with different authority can not be the same FS. 

Please review http://spark.apache.org/contributing.html before opening a 
pull request.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/merlintang/spark EAR-7377

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/19885.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 #19885


commit 3675f0a41fc0715d3d7122bbff3ab6d8fbe057c9
Author: Mingjie Tang 
Date:   2017-12-04T23:31:31Z

SPARK-22587 Spark job fails if fs.defaultFS and application jar are 
different url




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org