[SPARK-4075][SPARK-4434] Fix the URI validation logic for Application Jar name. #3326

sarutak · 2014-11-17T22:09:36Z

This PR adds a regression test for SPARK-4434.

SparkQA · 2014-11-17T22:15:33Z

Test build #23503 has started for PR 3326 at commit 6d4f47e.

This patch merges cleanly.

SparkQA · 2014-11-17T23:40:34Z

Test build #23503 has finished for PR 3326 at commit 6d4f47e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-17T23:40:38Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23503/
Test PASSed.

andrewor14 · 2014-11-18T00:15:38Z

core/src/test/scala/org/apache/spark/deploy/ClientSuite.scala

@@ -24,6 +24,7 @@ class ClientSuite extends FunSuite with Matchers {
  test("correctly validates driver jar URL's") {
    ClientArguments.isValidJarUrl("http://someHost:8080/foo.jar") should be (true)
    ClientArguments.isValidJarUrl("file://some/path/to/a/jarFile.jar") should be (true)


Not your change, but this should not be a valid jar URL right? My understanding is that one or three slashes are valid, but not two.

andrewor14 · 2014-11-18T00:16:46Z

Hey @sarutak also are you resubmitting your original change for SPARK-4434?

sarutak · 2014-11-18T00:20:30Z

O.K, I'll resubmit original change including this test case and fixing to handle double slashed file scheme.

…lash-testcase

In deploy.ClientArguments.isValidJarUrl, the url is checked as follows. def isValidJarUrl(s: String): Boolean = s.matches("(.+):(.+)jar") So, it allows like 'hdfs:file.jar' (no authority). Author: Kousuke Saruta <[email protected]> Closes apache#2925 from sarutak/uri-syntax-check-improvement and squashes the following commits: cf06173 [Kousuke Saruta] Improved URI syntax checking

SparkQA · 2014-11-18T01:00:32Z

Test build #23520 has started for PR 3326 at commit 9e09da2.

This patch merges cleanly.

SparkQA · 2014-11-18T02:22:22Z

Test build #23520 has finished for PR 3326 at commit 9e09da2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-18T02:22:25Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23520/
Test PASSed.

andrewor14 · 2014-11-18T02:37:54Z

Thanks, can you rename the title of the PR now that this is not just tests anymore?

sarutak · 2014-11-18T02:41:15Z

Thanks for pointing out. I've now modified.

vanzin · 2014-11-18T03:02:45Z

core/src/main/scala/org/apache/spark/deploy/ClientArguments.scala

@@ -73,7 +75,7 @@ private[spark] class ClientArguments(args: Array[String]) {

      if (!ClientArguments.isValidJarUrl(_jarUrl)) {
        println(s"Jar url '${_jarUrl}' is not in valid format.")
-        println(s"Must be a jar file path in URL format (e.g. hdfs://XX.jar, file://XX.jar)")
+        println(s"Must be a jar file path in URL format (e.g. hdfs://XX.jar, file:///XX.jar)")


If you're changing the file URL might as well change the hdfs one.

vanzin · 2014-11-18T03:08:34Z

Just nits w.r.t. naming of things, otherwise looks ok.

SparkQA · 2014-11-18T06:05:12Z

Test build #23538 has started for PR 3326 at commit 4f30210.

This patch merges cleanly.

SparkQA · 2014-11-18T06:06:24Z

Test build #23538 has finished for PR 3326 at commit 4f30210.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-18T06:06:25Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23538/
Test FAILed.

SparkQA · 2014-11-18T06:15:06Z

Test build #23539 has started for PR 3326 at commit c1c80ca.

This patch merges cleanly.

SparkQA · 2014-11-18T07:40:14Z

Test build #23539 has finished for PR 3326 at commit c1c80ca.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-18T07:40:17Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23539/
Test PASSed.

…lash-testcase

SparkQA · 2014-11-18T18:50:07Z

Test build #23552 has started for PR 3326 at commit 82bc9cc.

This patch merges cleanly.

andrewor14 · 2014-11-18T19:09:22Z

core/src/test/scala/org/apache/spark/deploy/ClientSuite.scala

-    ClientArguments.isValidJarUrl("file://some/path/to/a/jarFile.jar") should be (true)
+
+    // file scheme with authority and path is valid.
+    ClientArguments.isValidJarUrl("file://somehost/path/to/a/jarFile.jar") should be (true)


Wait, why is this valid?

andrewor14 · 2014-11-18T19:10:52Z

Hey @sarutak @vanzin I thought the following are valid:

file:/path/to/my.jar
file:///path/to/my.jar
hdfs://path/to/my.jar

but the following are not:

file://path/to/my.jar
hdfs:/path/to/my.jar
hdfs:///path/to/my.jar

Is this not the case?

sarutak · 2014-11-18T19:17:49Z

I think, double-slashed file scheme is valid in according to the specification of URI.
In the example URI you mentioned above (file://path/to/my.jar), path is recognized as authority.

Or, should we invalid double-slashed file scheme (file://), single-slashed any scheme(hdfs:/) and triple-slashed any scheme except for file scheme(hdfs:///) regardless of the URI specification?

vanzin · 2014-11-18T19:19:35Z

@andrewor14 all the URIs you mention as not valid are actually valid.

file scheme with authority is valid. It won't do anything useful in most cases, but on Windows, for example, it might work (if it's pointing at an SMB share)
hdfs scheme with no host will probably default to fs.defaultFS, if I remember correctly the HDFS code. (if the scheme matches, otherwise HDFS will throw an exception)
/// means "default host", which is generally "localhost" but in HDFS might be the above.

So those are all valid URIs, which is why no exception is thrown when you construct the URI object.

andrewor14 · 2014-11-18T20:10:59Z

I see. By "valid" here we mean the URI provides some combination of the scheme, the path and the authority. However, when you do two-slashes on file:, it actually interprets it differently from what I would otherwise assume.

scala> new URI("file:/path/to/my.jar").getPath
res9: String = /path/to/my.jar

scala> new URI("file://path/to/my.jar").getPath
res10: String = /to/my.jar

I suppose then it is the user's responsibility to not make this mistake. We can't prevent this because "path" could be a valid authority or hostname here and we won't be able to tell the difference.

vanzin · 2014-11-18T20:13:20Z

However, when you do two-slashes on file:, it actually interprets it differently from what I would otherwise assume

That's because you assume wrong. :-) "//" is the indication that the next component is the authority. You can omit the authority by not specifying "//". "file" is not special. It works just like any other URI (http, hdfs or otherwise).

SparkQA · 2014-11-18T20:13:52Z

Test build #23552 has finished for PR 3326 at commit 82bc9cc.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-18T20:13:55Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23552/
Test PASSed.

andrewor14 · 2014-11-18T20:16:53Z

Ok, I'm merging this into master and 1.2. Not going to merge this into 1.1 because the benefit here is not worth potentially causing another regression.

… Jar name. This PR adds a regression test for SPARK-4434. Author: Kousuke Saruta <[email protected]> Closes apache#3326 from sarutak/add-triple-slash-testcase and squashes the following commits: 82bc9cc [Kousuke Saruta] Fixed wrong grammar in comment 9149027 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into add-triple-slash-testcase c1c80ca [Kousuke Saruta] Fixed style 4f30210 [Kousuke Saruta] Modified comments 9e09da2 [Kousuke Saruta] Fixed URI validation for jar file d4b99ef [Kousuke Saruta] [SPARK-4075] [Deploy] Jar url validation is not enough for Jar file ac79906 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into add-triple-slash-testcase 6d4f47e [Kousuke Saruta] Added a test case as a regression check for SPARK-4434

… Jar name. This PR adds a regression test for SPARK-4434. Author: Kousuke Saruta <[email protected]> Closes #3326 from sarutak/add-triple-slash-testcase and squashes the following commits: 82bc9cc [Kousuke Saruta] Fixed wrong grammar in comment 9149027 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into add-triple-slash-testcase c1c80ca [Kousuke Saruta] Fixed style 4f30210 [Kousuke Saruta] Modified comments 9e09da2 [Kousuke Saruta] Fixed URI validation for jar file d4b99ef [Kousuke Saruta] [SPARK-4075] [Deploy] Jar url validation is not enough for Jar file ac79906 [Kousuke Saruta] Merge branch 'master' of git://git.apache.org/spark into add-triple-slash-testcase 6d4f47e [Kousuke Saruta] Added a test case as a regression check for SPARK-4434 (cherry picked from commit bfebfd8) Signed-off-by: Andrew Or <[email protected]>

andrewor14 · 2014-11-19T20:15:09Z

Hey @sarutak mind closing this? It's already merged

sarutak · 2014-11-19T20:28:44Z

Thanks for notification. I close this PR.

Added a test case as a regression check for SPARK-4434

6d4f47e

sarutak mentioned this pull request Nov 17, 2014

[SPARK-4434] spark-submit cluster deploy mode JAR URLs are broken (For branch-1.1) #3295

Closed

andrewor14 reviewed Nov 18, 2014
View reviewed changes

sarutak added 3 commits November 17, 2014 16:23

Merge branch 'master' of git://git.apache.org/spark into add-triple-s…

ac79906

…lash-testcase

Fixed URI validation for jar file

9e09da2

sarutak changed the title ~~[SPARK-4434] Add a test case as a regression check for SPARK-4434~~ [SPARK-4075][SPARK-4434] Fix the URI validation logic for Application Jar name. Nov 18, 2014

vanzin reviewed Nov 18, 2014
View reviewed changes

Modified comments

4f30210

Fixed style

c1c80ca

Merge branch 'master' of git://git.apache.org/spark into add-triple-s…

9149027

…lash-testcase

Fixed wrong grammar in comment

82bc9cc

andrewor14 reviewed Nov 18, 2014
View reviewed changes

sarutak closed this Nov 19, 2014

sarutak deleted the add-triple-slash-testcase branch April 11, 2015 05:20

[SPARK-4075][SPARK-4434] Fix the URI validation logic for Application Jar name. #3326

[SPARK-4075][SPARK-4434] Fix the URI validation logic for Application Jar name. #3326

Uh oh!

Conversation

sarutak commented Nov 17, 2014

Uh oh!

SparkQA commented Nov 17, 2014

Uh oh!

SparkQA commented Nov 17, 2014

Uh oh!

AmplabJenkins commented Nov 17, 2014

Uh oh!

andrewor14 Nov 18, 2014

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Nov 18, 2014

Uh oh!

sarutak commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

AmplabJenkins commented Nov 18, 2014

Uh oh!

andrewor14 commented Nov 18, 2014

Uh oh!

sarutak commented Nov 18, 2014

Uh oh!

vanzin Nov 18, 2014

Choose a reason for hiding this comment

Uh oh!

vanzin commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

AmplabJenkins commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

AmplabJenkins commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

andrewor14 Nov 18, 2014

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Nov 18, 2014

Uh oh!

sarutak commented Nov 18, 2014

Uh oh!

vanzin commented Nov 18, 2014

Uh oh!

andrewor14 commented Nov 18, 2014

Uh oh!

vanzin commented Nov 18, 2014

Uh oh!

SparkQA commented Nov 18, 2014

Uh oh!

AmplabJenkins commented Nov 18, 2014

Uh oh!

andrewor14 commented Nov 18, 2014

Uh oh!

andrewor14 commented Nov 19, 2014

Uh oh!

sarutak commented Nov 19, 2014

Uh oh!

Uh oh!