[SPARK-26180][CORE][TEST] Reuse withTempDir function to the SparkCore test case #23151

heary-cao · 2018-11-27T03:27:29Z

What changes were proposed in this pull request?

Currently, the common withTempDir function is used in Spark SQL test cases. To handle val dir = Utils. createTempDir() and Utils. deleteRecursively (dir). Unfortunately, the withTempDir function cannot be used in the Spark Core test case. This PR Sharing withTempDir function in Spark Sql and SparkCore to clean up SparkCore test cases. thanks.

How was this patch tested?

N / A

SparkQA · 2018-11-27T03:34:08Z

Test build #99302 has finished for PR 23151 at commit d29d4ec.

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

SparkQA · 2018-11-27T08:05:01Z

Test build #99303 has finished for PR 23151 at commit 6402d2d.

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

heary-cao · 2018-11-27T08:57:51Z

retest this please

SparkQA · 2018-11-27T12:56:51Z

Test build #99315 has finished for PR 23151 at commit 6402d2d.

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

heary-cao · 2018-11-28T00:42:37Z

cc @cloud-fan, @kiszk, @dongjoon-hyun

kiszk · 2018-11-28T01:32:39Z

Good catch

kiszk · 2018-11-28T01:33:28Z

core/src/test/scala/org/apache/spark/SparkFunSuite.scala

+   *
+   * @todo Probably this method should be moved to a more general place
+   */
+  protected def withCreateTempDir(f: File => Unit): Unit = {


Is there any reason not to use withTempDir as a function name like other modules?

trait SQLTestUtils extends SparkFunSuite with SQLTestUtilsBase with PlanTest
if SparkFunSuite and SQLTestUtilsBase use the same name withTempDir.
Can cause name contamination? thanks.

if we have this function in SparkFunSuite, why do we need to define it again in SQLTestUtils?

Currently, withTempDir and withCreateTempDir are somewhat different.
withTempDir

protected def withTempDir(f: File => Unit): Unit = { val dir = Utils.createTempDir().getCanonicalFile try f(dir) finally { // wait for all tasks to finish before deleting files waitForTasksToFinish() Utils.deleteRecursively(dir) } } protected def waitForTasksToFinish(): Unit = { eventually(timeout(10.seconds)) { assert(spark.sparkContext.statusTracker .getExecutorInfos.map(_.numRunningTasks()).sum == 0) } }

withCreateTempDir

protected def withCreateTempDir(f: File => Unit): Unit = { val dir = Utils.createTempDir() try f(dir) finally { Utils.deleteRecursively(dir) } }

thanks.

shall we also do waitForTasksToFinish in withCreateTempDir?

I'm not sure if I need assert(spark.sparkContext.statusTracker .getExecutorInfos.map(_.numRunningTasks()).sum == 0). after all, protected def spark: SparkSession defined in SQLTestData. Unless we construct one waitForTasksToFinish look like

protected def waitForTasksToFinish(): Unit = { eventually(timeout(10.seconds)) { } }

OK, how about in SQLTestUtils we override withTempDir with this extra logic?

Sorry, maybe I didn't make it clear. withTempDir define in SQLTestUtilsBase not in SQLTestUtils .if we will override withTempDir first must be remove withTempDir from SQLTestUtilsBase to SQLTestUtils and override, but in this way, other functions are also involved.

I'm not talking about details like which calss to override, just the idea. Why wouldn't override work?

Yes, it seems like we should be able to use an override. The subclass that needs to inject an additional method call in the block can call the super method with a lambda that calls the user-supplied block, then this other method. It's probably worth whatever surgery is needed to make this clean and reduce duplication. We already have a lot of "create temp thing" methods all over.

kiszk · 2018-11-28T01:33:51Z

core/src/test/scala/org/apache/spark/SparkFunSuite.scala

+   * @todo Probably this method should be moved to a more general place
+   */
+  protected def withCreateTempDir(f: File => Unit): Unit = {
+    val dir = Utils.createTempDir()


Is it better to call .getCanonicalFile, too?

I'm not sure if I need call .getCanonicalFile again. i feel it's a little redundant. review Utils.createTempDir() --> createDirectory --> dir.getCanonicalFile.
It has been called .getCanonicalFile. please correct If I misunderstand. thanks.

Yes shouldn't be necessary here.

cloud-fan · 2018-11-28T12:16:29Z

core/src/test/scala/org/apache/spark/SparkFunSuite.scala

+   * Creates a temporary directory, which is then passed to `f` and will be deleted after `f`
+   * returns.
+   *
+   * @todo Probably this method should be moved to a more general place


I think this is the most general place we can find...

SparkQA · 2018-11-29T06:03:24Z

Test build #99421 has finished for PR 23151 at commit d7482a3.

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

SparkQA · 2018-11-29T07:30:36Z

Test build #99436 has finished for PR 23151 at commit 073f5f7.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-29T08:33:34Z

Test build #99440 has finished for PR 23151 at commit 0537eb6.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-29T08:53:52Z

Test build #99441 has finished for PR 23151 at commit 819dc6a.

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

SparkQA · 2018-11-29T13:21:24Z

Test build #99442 has finished for PR 23151 at commit c387cc7.

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

srowen

I like the cleanup. I think there are even more places where createTempDir() is called and could use this new pattern. If you have time, are you willing to search out some more? we don't need to get all of them but maybe some more easy cases out there.

srowen · 2018-11-29T16:26:09Z

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

    val unusedJar = TestUtils.createJarWithClasses(Seq.empty)
    val fileSystem = Utils.getHadoopFileSystem("/",
      SparkHadoopUtil.get.newConfiguration(new SparkConf()))
-    try {
+    withTempDir { testDir =>
+      testDir.deleteOnExit()


Although I think this is redundant for temp dirs, you can put this in the Utils.createTempDir method and take it out in places like this.

deleteOnExit is redundant for directories created by Utils.createTempDir. That method already tracks directories to cleanup on exit.

(Also deleteOnExit does not work for non-empty directories.)

srowen · 2018-11-29T16:26:59Z

sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala

+   */
+  protected override def withTempDir(f: File => Unit): Unit = {
+    val dir = Utils.createTempDir().getCanonicalFile
+    try f(dir) finally {


Why not call the super method with a function that calls f, then waitForTasksToFinish()?

cloud-fan · 2018-11-30T03:00:04Z

sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala

+  /**
+   * Creates a temporary directory, which is then passed to `f` and will be deleted after `f`
+   * returns.
+   *


nit: unnecessary blank line

cloud-fan · 2018-11-30T03:00:34Z

sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala

+   *
+   */
+  protected override def withTempDir(f: File => Unit): Unit = {
+    super.withTempDir { dir =>


yea this is what I expect, thanks for doing it!

HyukjinKwon · 2018-11-30T04:50:19Z

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


-    // All the resources should still be remote paths, so that YARN client will not upload again.
-    conf.get("spark.yarn.dist.jars") should be (tmpJarPath)


I wouldn't change those spaces alone tho. Let's leave as were.

HyukjinKwon · 2018-11-30T04:52:13Z

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


-    conf.get(PY_FILES.key) should be (s"s3a://${pyFile.getAbsolutePath}")
-    conf.get("spark.submit.pyFiles") should (startWith("/"))
+      conf.get(PY_FILES.key) should be(s"s3a://${pyFile.getAbsolutePath}")


ditto. Technically it should better be assert and avoid infix notation but I think we don't have to do it here.

HyukjinKwon

Looks good. PR description should also be updated (e.g., inheritance, same name, etc.)

SparkQA · 2018-11-30T05:48:29Z

Test build #99489 has finished for PR 23151 at commit 482c4f4.

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

SparkQA · 2018-11-30T08:05:02Z

Test build #99496 has finished for PR 23151 at commit beccd74.

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

HyukjinKwon · 2018-11-30T09:10:27Z

retest this please

SparkQA · 2018-11-30T13:23:01Z

Test build #99505 has finished for PR 23151 at commit beccd74.

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

srowen

I think the comments to date have been incorporated... this is looking good.

dongjoon-hyun · 2018-11-30T19:34:31Z

core/src/test/scala/org/apache/spark/SparkContextSuite.scala

@@ -245,8 +245,7 @@ class SparkContextSuite extends SparkFunSuite with LocalSparkContext with Eventu
  }

  test("cannot call addFile with different paths that have the same filename") {


Hi, @heary-cao . This PR is very useful. While touching this file, could you replace more val dir = Utils.createTempDir() with new withTempDir in this file? For example, the above test case at line 235.

ok, i have update it. please review it. thanks.

SparkQA · 2018-12-01T02:50:06Z

Test build #99532 has finished for PR 23151 at commit de0ef11.

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

SparkQA · 2018-12-01T08:07:20Z

Test build #99533 has finished for PR 23151 at commit 6d2f2d3.

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

HyukjinKwon · 2018-12-01T08:33:50Z

Merged to master.

… test case ## What changes were proposed in this pull request? Currently, the common `withTempDir` function is used in Spark SQL test cases. To handle `val dir = Utils. createTempDir()` and `Utils. deleteRecursively (dir)`. Unfortunately, the `withTempDir` function cannot be used in the Spark Core test case. This PR Sharing `withTempDir` function in Spark Sql and SparkCore to clean up SparkCore test cases. thanks. ## How was this patch tested? N / A Closes apache#23151 from heary-cao/withCreateTempDir. Authored-by: caoxuewen <cao.xuewen@zte.com.cn> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

heary-cao force-pushed the withCreateTempDir branch from d29d4ec to 6402d2d Compare November 27, 2018 04:11

kiszk reviewed Nov 28, 2018

View reviewed changes

cloud-fan reviewed Nov 28, 2018

View reviewed changes

heary-cao force-pushed the withCreateTempDir branch from 6402d2d to d7482a3 Compare November 29, 2018 01:58

heary-cao force-pushed the withCreateTempDir branch from d7482a3 to 073f5f7 Compare November 29, 2018 07:22

heary-cao force-pushed the withCreateTempDir branch from 073f5f7 to 0537eb6 Compare November 29, 2018 08:24

heary-cao force-pushed the withCreateTempDir branch from 0537eb6 to 819dc6a Compare November 29, 2018 08:46

heary-cao force-pushed the withCreateTempDir branch from 819dc6a to c387cc7 Compare November 29, 2018 09:06

srowen reviewed Nov 29, 2018

View reviewed changes

heary-cao force-pushed the withCreateTempDir branch from c387cc7 to 482c4f4 Compare November 30, 2018 01:40

cloud-fan reviewed Nov 30, 2018

View reviewed changes

cloud-fan approved these changes Nov 30, 2018

View reviewed changes

HyukjinKwon reviewed Nov 30, 2018

View reviewed changes

HyukjinKwon approved these changes Nov 30, 2018

View reviewed changes

heary-cao changed the title ~~[SPARK-26180][CORE][TEST] Add a withCreateTempDir function to the SparkCore test case~~ [SPARK-26180][CORE][TEST] Reuse withTempDir function to the SparkCore test case Nov 30, 2018

heary-cao force-pushed the withCreateTempDir branch from 482c4f4 to beccd74 Compare November 30, 2018 05:56

srowen approved these changes Nov 30, 2018

View reviewed changes

dongjoon-hyun reviewed Nov 30, 2018

View reviewed changes

heary-cao force-pushed the withCreateTempDir branch from beccd74 to de0ef11 Compare December 1, 2018 02:45

Add a withCreateTempDir function to the SparkCore test case

6d2f2d3

heary-cao force-pushed the withCreateTempDir branch from de0ef11 to 6d2f2d3 Compare December 1, 2018 03:44

asfgit closed this in 327ac83 Dec 1, 2018


		// All the resources should still be remote paths, so that YARN client will not upload again.
		conf.get("spark.yarn.dist.jars") should be (tmpJarPath)

		@@ -245,8 +245,7 @@ class SparkContextSuite extends SparkFunSuite with LocalSparkContext with Eventu
		}

		test("cannot call addFile with different paths that have the same filename") {

[SPARK-26180][CORE][TEST] Reuse withTempDir function to the SparkCore test case #23151

[SPARK-26180][CORE][TEST] Reuse withTempDir function to the SparkCore test case #23151

Conversation

heary-cao commented Nov 27, 2018 • edited

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Nov 27, 2018

SparkQA commented Nov 27, 2018

heary-cao commented Nov 27, 2018

SparkQA commented Nov 27, 2018

heary-cao commented Nov 28, 2018

kiszk commented Nov 28, 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

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

heary-cao Nov 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 29, 2018

SparkQA commented Nov 29, 2018

SparkQA commented Nov 29, 2018

SparkQA commented Nov 29, 2018

SparkQA commented Nov 29, 2018

srowen 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

HyukjinKwon left a comment

Choose a reason for hiding this comment

SparkQA commented Nov 30, 2018

SparkQA commented Nov 30, 2018

HyukjinKwon commented Nov 30, 2018

SparkQA commented Nov 30, 2018

srowen left a comment

Choose a reason for hiding this comment

dongjoon-hyun Nov 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 1, 2018

SparkQA commented Dec 1, 2018

HyukjinKwon commented Dec 1, 2018

heary-cao commented Nov 27, 2018 •

edited

heary-cao Nov 28, 2018 •

edited

dongjoon-hyun Nov 30, 2018 •

edited