[GitHub] [spark] JoshRosen commented on a change in pull request #34013: [SPARK-36774][CORE][TESTS] Move SparkSubmitTestUtils to core module and use it in SparkSubmitSuite

[GitBox <gi...@apache.org>]

JoshRosen commented on a change in pull request #34013:
URL: https://github.com/apache/spark/pull/34013#discussion_r709823439



##########
File path: core/src/test/scala/org/apache/spark/deploy/SparkSubmitTestUtils.scala
##########
@@ -25,33 +25,36 @@ import scala.collection.mutable.ArrayBuffer
 
 import org.scalatest.concurrent.{Signaler, ThreadSignaler, TimeLimits}
 import org.scalatest.exceptions.TestFailedDueToTimeoutException
+import org.scalatest.time.Span
 import org.scalatest.time.SpanSugar._
 
+import org.apache.spark.ProcessTestUtils.ProcessOutputCapturer
 import org.apache.spark.SparkFunSuite
-import org.apache.spark.sql.test.ProcessTestUtils.ProcessOutputCapturer
 import org.apache.spark.util.Utils
 
 trait SparkSubmitTestUtils extends SparkFunSuite with TimeLimits {
 
+  protected val defaultSparkSubmitTimeout: Span = 1.minute
+
   // Necessary to make ScalaTest 3.x interrupt a thread on the JVM like ScalaTest 2.2.x
   implicit val defaultSignaler: Signaler = ThreadSignaler
 
   // NOTE: This is an expensive operation in terms of time (10 seconds+). Use sparingly.
-  // This is copied from org.apache.spark.deploy.SparkSubmitSuite
   protected def runSparkSubmit(
       args: Seq[String],
       sparkHomeOpt: Option[String] = None,
+      timeout: Span = defaultSparkSubmitTimeout,
       isSparkTesting: Boolean = true): Unit = {
     val sparkHome = sparkHomeOpt.getOrElse(
       sys.props.getOrElse("spark.test.home", fail("spark.test.home is not set!")))
     val history = ArrayBuffer.empty[String]
     val sparkSubmit = if (Utils.isWindows) {
       // On Windows, `ProcessBuilder.directory` does not change the current working directory.
-      new File("..\\..\\bin\\spark-submit.cmd").getAbsolutePath
+      new File(new File(sparkHome, "bin"), "spark-submit.cmd")
     } else {
-      "./bin/spark-submit"
+      new File(new File(sparkHome, "bin"), "spark-submit")
     }
-    val commands = Seq(sparkSubmit) ++ args
+    val commands = Seq(sparkSubmit.getCanonicalPath) ++ args

Review comment:
       Here's my reasoning on why this test-utility-only behavior change is okay:
   
   1. [`SparkSubmitSuite`'s version of `runSparkSubmit`](https://github.com/apache/spark/blob/afd406e4d0b9701d3733ac8afe44339661fb4853/core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala#L1536-L1544) was already calling `getCanonicalPath`.
   
      In `getCanonicalPath`, paths are resolved relative to the test JVM's current working directory, whose depth from `SPARK_HOME` differs across submodules (it's `../` in `core` vs `../../` in `sql/hive`). If I wanted to preserve this path resolution strategy then I'd need to add a way to customize the `$root` prefix in different suites and that feels error-prone and clunky. It's much simpler to just specify a path relative to `sparkHome`.
      
      Given this, I think this change is correct w.r.t. matching the `SparkSubmitSuite` behavior.
   
   2. The original `SparkSubmitTestUtils` didn't use canonical paths. Instead, it relied on being able to change the process builder's working directory. In an old PR, it looks like @HyukjinKwon originally considered using canonical or absolute paths but stuck with the relative paths out of concern that the change might (possibly) have caused `HiveSparkSubmitSuite` to become flaky: https://github.com/apache/spark/pull/16451/files#diff-b71d15c52a3d16c9e69cb230aefe4e0ef1a00eb459ee84e08f9e63f39ddf3525. If we don't see any new evidence of flakiness then I think we should proceed with unifying on this PR's simpler approach.
   
   3.  [HiveExternalCatalogVersionsSuite calls `runSparkSubmit` with a `sparkHome` pointing to a downloaded old version of Spark](https://github.com/apache/spark/blob/c21779729765f640272e0249c6e71145aa5ccfdf/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala#L219). The purpose of that code is to create testing warehouses with old versions of Spark, so I believe the intent is to invoke the version of `spark-submit` bundled in those versions. With the old version of this code, the behavior would differ across OS's: windows builds would invoke the version of `spark-submit` in the Spark repository (because the process builder working directory would be ignored), whereas non-windows builds would invoke the `spark-submit` in the downloaded Spark. Despite this, things still worked correctly because the `spark-submit` script [invokes the copy of `spark-class` found in `$SPARK_HOME`](https://github.com/apache/spark/blob/c21779729765f640272e0249c6e71145aa5
 ccfdf/bin/spark-submit#L27). That's a bit confusing, though, so I think it's better to just use absolute paths as I've done here.
   
   I'm going to remove the
   
   ```
   // On Windows, `ProcessBuilder.directory` does not change the current working directory.
   ```
   
   comment since I think it's now superfluous.




-- 
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