[SPARK-23729][CORE] Respect URI fragment when resolving globs #20853

misutoth · 2018-03-18T06:14:37Z

What changes were proposed in this pull request?

Firstly, glob resolution will not result in swallowing the remote name part (that is preceded by the # sign) in case of --files or --archives options

Moreover in the special case of multiple resolutions when the remote naming does not make sense and error is returned.

How was this patch tested?

Enhanced current test and wrote additional test for the error case

…hich is meant to be the remote name In case glob resolution results multiple items for a file with a remote name, an error is displayed.

squito · 2018-03-19T16:58:26Z

Jenkins, ok to test

vanzin · 2018-03-19T17:50:15Z

nit: pr title should describe the solution, not the problem. e.g. "Respect URI fragment when resolving globs" is a description of the solution.

vanzin · 2018-03-19T17:57:40Z

Also this is not related to structured streaming, so [ss] is wrong. This is a core change.

gaborgsomogyi

Please change [SS] which is structured streaming to something else like [CORE].

gaborgsomogyi · 2018-03-19T17:17:05Z

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

@@ -105,11 +105,17 @@ class SparkSubmitSuite

  // Necessary to make ScalaTest 3.x interrupt a thread on the JVM like ScalaTest 2.2.x
  implicit val defaultSignaler: Signaler = ThreadSignaler
+  var dir: File = null


Most of the tests doesn't use this dir at all. Why create it for all the tests?

I was thinking about this too. I wanted to avoid making it an Option or doing a not very nice null check. I can do that later though...

I mean more like put something here which is used by more than 2 tests. There are ~40 tests which are just creating and deleting this directory without any benefit.

I wanted to make sure the directory is deleted even if the test fails

Yeah, there is a good example here: test("launch simple application with spark-submit with redaction")

gaborgsomogyi · 2018-03-19T17:32:19Z

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

    val jars = "/jar1,/jar2"                 // --jars
    val files = "local:/file1,file2"          // --files
-    val archives = "file:/archive1,archive2" // --archives
+    val archives = s"file:/archive1,${dir.toPath.toAbsolutePath.toString}/*.zip#archive3"
+                                             // --archives
    val pyFiles = "py-file1,py-file2"        // --py-files


Does --py-files support renaming?

According to the doc only --files and --archives support it.

OK, thanks.

YARN's Client.scala supports renaming for everything that uses the distributed cache, even if that's not explicitly called out in the docs.

gaborgsomogyi · 2018-03-19T17:34:00Z

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

@@ -657,6 +667,31 @@ class SparkSubmitSuite
    conf3.get(PYSPARK_PYTHON.key) should be ("python3.5")
  }

+  var cleanExit = false


What is it used for?

gaborgsomogyi · 2018-03-19T17:38:50Z

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

+    } catch {
+      case e: SparkException =>
+        printErrorAndExit(e.getMessage)
+        throw new RuntimeException("Unreachable production code")


Nit: I have a feeling it's a bit overkill compared to the other occurences.

Which part do you mean and overkill?

throw new RuntimeException...

Otherwise execution just continues in the test itself where exitFn does not stop it.

Maybe just let the exception propagate? That's what a lot of this code does... then you don't need to change this file at all.

Actually the directory deletion is hooked into JVM shutdown. So I will let this to do the housekeeping for us and will avoid a new field either.

gaborgsomogyi · 2018-03-19T17:55:15Z

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

+      val renameAs = if (spath.length > 1) Some(spath(1)) else None
+      val resolved: Array[String] = resoloveGlobPath(spath(0), hadoopConf)
+      resolved match {
+        case array: Array[String] if !renameAs.isEmpty && array.length>1 =>


Nit: array.length > 1

gaborgsomogyi · 2018-03-19T18:07:49Z

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

+      val spath = path.split('#')
+      val renameAs = if (spath.length > 1) Some(spath(1)) else None
+      val resolved: Array[String] = resoloveGlobPath(spath(0), hadoopConf)
+      resolved match {


This can be simplified something like this: (renameAs, resolved) match...

This whole match block is a little ugly, but I'll wait to see how you implement Gabor's suggestion...

gaborgsomogyi · 2018-03-19T18:37:28Z

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

-          }.getOrElse(Array(path))
+      val spath = path.split('#')
+      val renameAs = if (spath.length > 1) Some(spath(1)) else None
+      val resolved: Array[String] = resoloveGlobPath(spath(0), hadoopConf)


Nit: resolveGlobPath

gaborgsomogyi · 2018-03-19T18:41:41Z

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

+        case array: Array[String] if !renameAs.isEmpty && array.length>1 =>
+          throw new SparkException(
+            s"${spath(1)} resolves ambiguously to multiple files: ${array.mkString(",")}")
+        case array: Array[String] if !renameAs.isEmpty => array.map( _ + "#" + renameAs.get)


Maybe we can find some meaningful name for array which makes me hard to read the code.

vanzin · 2018-03-19T19:09:04Z

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

-          Option(fs.globStatus(new Path(uri))).map { status =>
-            status.filter(_.isFile).map(_.getPath.toUri.toString)
-          }.getOrElse(Array(path))
+      val spath = path.split('#')


Why not use Utils.resolveURI as before? Parsing URIs by hand is very sketchy.

You are right. It took some time to clone a URI without the fragment part though but next version will include that.

vanzin · 2018-03-19T19:13:06Z

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

+      val spath = path.split('#')
+      val renameAs = if (spath.length > 1) Some(spath(1)) else None
+      val resolved: Array[String] = resoloveGlobPath(spath(0), hadoopConf)
+      resolved match {


This whole match block is a little ugly, but I'll wait to see how you implement Gabor's suggestion...

vanzin · 2018-03-19T19:14:33Z

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

+    } catch {
+      case e: SparkException =>
+        printErrorAndExit(e.getMessage)
+        throw new RuntimeException("Unreachable production code")


Maybe just let the exception propagate? That's what a lot of this code does... then you don't need to change this file at all.

misutoth · 2018-03-19T20:32:21Z

Maybe just let the exception propagate? That's what a lot of this code does... then you don't need to change this file at all.

@vanzin I want to present an error on the CLI. This is what the printErrorAndExit does as I understood.

On the other hand I simplified to just rethrowing the Exception e

SparkQA · 2018-03-19T20:58:18Z

Test build #88379 has finished for PR 20853 at commit 50b5ad1.

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

gaborgsomogyi · 2018-03-19T21:06:08Z

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

-          }.getOrElse(Array(path))
+      val (base, fragment) = splitOnFragment(Utils.resolveURI(path))
+      (resolveGlobPath(base, hadoopConf), fragment) match {
+        case (resolved: Array[String], Some(_)) if resolved.length > 1 => throw new SparkException(


Type inference is not working here?

gaborgsomogyi · 2018-03-19T21:06:19Z

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

+      (resolveGlobPath(base, hadoopConf), fragment) match {
+        case (resolved: Array[String], Some(_)) if resolved.length > 1 => throw new SparkException(
+            s"${base.toString} resolves ambiguously to multiple files: ${resolved.mkString(",")}")
+        case (resolved: Array[String], Some(namedAs)) => resolved.map( _ + "#" + namedAs)


…small improvements

SparkQA · 2018-03-20T00:57:16Z

Test build #88385 has finished for PR 20853 at commit 9f391de.

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

SparkQA · 2018-03-20T03:10:21Z

Test build #88389 has finished for PR 20853 at commit 8a12452.

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

jiangxb1987 · 2018-03-20T06:07:30Z

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

+      val (base, fragment) = splitOnFragment(path)
+      (resolveGlobPath(base, hadoopConf), fragment) match {
+        case (resolved, Some(_)) if resolved.length > 1 => throw new SparkException(
+            s"${base.toString} resolves ambiguously to multiple files: ${resolved.mkString(",")}")


nit: resolved.mkString(", ")

There was no space used here before. Actually there should not be any space in the resulting list. Tests also rely on this.

jiangxb1987 · 2018-03-20T06:08:30Z

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

+    (withoutFragment, fragment)
+  }
+
+  private def resolveGlobPath(uri: URI, hadoopConf: Configuration): Array [String] = {


nit: Array[String]

jiangxb1987 · 2018-03-20T06:09:02Z

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

+    try {
+      doPrepareSubmitEnvironment(args, conf)
+    } catch {
+      case e: SparkException => printErrorAndExit(e.getMessage); throw e


nit:

case e: SparkException => printErrorAndExit(e.getMessage) throw e

SparkQA · 2018-03-20T07:05:01Z

Test build #88406 has finished for PR 20853 at commit 5515da6.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-03-20T17:27:23Z

retest this please

SparkQA · 2018-03-20T21:19:05Z

Test build #88430 has finished for PR 20853 at commit 5515da6.

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

SparkQA · 2018-03-20T21:21:43Z

Test build #88429 has finished for PR 20853 at commit 5515da6.

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

vanzin

Some minor things otherwise looks good.

vanzin · 2018-03-21T16:40:01Z

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

+  private def splitOnFragment(path: String): (URI, Option[String]) = {
+    val uri = Utils.resolveURI(path)
+    val withoutFragment = new URI(uri.getScheme, uri.getSchemeSpecificPart, null)
+    val fragment = if (uri.getFragment != null) Some(uri.getFragment) else None


Option(uri.getFragment)

vanzin · 2018-03-21T16:45:49Z

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

+    Files.createFile(archive2)
+    val jars = "/jar1,/jar2"                 // --jars
+    val files = "local:/file1,file2"          // --files
+    // --archives


Unnecessary comment. I know the other test has them, but I'd just remove these from this new code, since they don't add any useful information.

vanzin · 2018-03-21T16:46:18Z

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

+    val archives = s"file:/archive1,${dir.toPath.toAbsolutePath.toString}/*.zip#archive3"
+    val pyFiles = "py-file1,py-file2"        // --py-files
+
+    // Test files and archives (Yarn)


Unnecessary comment.

…ation

SparkQA · 2018-03-21T21:15:44Z

Test build #88476 has finished for PR 20853 at commit ce5273d.

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

vanzin · 2018-03-22T00:05:19Z

Merging to master / 2.3.

Firstly, glob resolution will not result in swallowing the remote name part (that is preceded by the `#` sign) in case of `--files` or `--archives` options Moreover in the special case of multiple resolutions when the remote naming does not make sense and error is returned. Enhanced current test and wrote additional test for the error case Author: Mihaly Toth <misutoth@gmail.com> Closes #20853 from misutoth/glob-with-remote-name. (cherry picked from commit 0604bea) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

Firstly, glob resolution will not result in swallowing the remote name part (that is preceded by the `#` sign) in case of `--files` or `--archives` options Moreover in the special case of multiple resolutions when the remote naming does not make sense and error is returned. Enhanced current test and wrote additional test for the error case Author: Mihaly Toth <misutoth@gmail.com> Closes apache#20853 from misutoth/glob-with-remote-name. (cherry picked from commit 0604bea) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

Ref: HADOOP-42709 Firstly, glob resolution will not result in swallowing the remote name part (that is preceded by the `#` sign) in case of `--files` or `--archives` options Moreover in the special case of multiple resolutions when the remote naming does not make sense and error is returned. Enhanced current test and wrote additional test for the error case Author: Mihaly Toth <misutoth@gmail.com> Closes apache#20853 from misutoth/glob-with-remote-name. (cherry picked from commit 0604bea) RB=1500362 BUG=LIHADOOP-42709 G=superfriends-reviewers R=fli,mshen,yezhou,edlu A=fli,xhzhang

[SPARK-23729][SS] Glob resolution is done without the fragment part w…

50b5ad1

…hich is meant to be the remote name In case glob resolution results multiple items for a file with a remote name, an error is displayed.

misutoth changed the title ~~[SPARK-23729][SS] Glob resolution is done without the fragment part which is meant to be the remote name~~ [SPARK-23729][SS] Respect URI fragment when resolving globs Mar 19, 2018

misutoth changed the title ~~[SPARK-23729][SS] Respect URI fragment when resolving globs~~ [SPARK-23729][CORE] Respect URI fragment when resolving globs Mar 19, 2018

gaborgsomogyi requested changes Mar 19, 2018

View reviewed changes

gaborgsomogyi reviewed Mar 19, 2018

View reviewed changes

vanzin reviewed Mar 19, 2018

View reviewed changes

[SPARK-23729][CORE] Refactors based on review comments

9f391de

gaborgsomogyi reviewed Mar 19, 2018

View reviewed changes

[SPARK-23729][CORE] Simplified by utilizing type inference, and some …

8a12452

…small improvements

jiangxb1987 reviewed Mar 20, 2018

View reviewed changes

[SPARK-23729][CORE] Fixed a few nit issues

5515da6

vanzin reviewed Mar 21, 2018

View reviewed changes

[SPARK-23729][CORE] Removed unnecessary comment, simlified option cre…

ce5273d

…ation

asfgit closed this in 0604bea Mar 22, 2018

[SPARK-23729][CORE] Respect URI fragment when resolving globs #20853

[SPARK-23729][CORE] Respect URI fragment when resolving globs #20853

Conversation

misutoth commented Mar 18, 2018

What changes were proposed in this pull request?

How was this patch tested?

squito commented Mar 19, 2018

vanzin commented Mar 19, 2018

vanzin commented Mar 19, 2018

gaborgsomogyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misutoth commented Mar 19, 2018

SparkQA commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 20, 2018

SparkQA commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 20, 2018

vanzin commented Mar 20, 2018

SparkQA commented Mar 20, 2018

SparkQA commented Mar 20, 2018

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 21, 2018

vanzin commented Mar 22, 2018