SPARK-6338 [CORE] Use standard temp dir mechanisms in tests to avoid orphaned temp files #5029

srowen · 2015-03-14T21:25:11Z

Use Utils.createTempDir() to replace other temp file mechanisms used in some tests, to further ensure they are cleaned up, and simplify

SparkQA · 2015-03-14T21:28:06Z

Test build #28614 has started for PR 5029 at commit 1a12efa.

This patch merges cleanly.

SparkQA · 2015-03-14T22:51:57Z

Test build #28614 has finished for PR 5029 at commit 1a12efa.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-14T22:52:01Z

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

SparkQA · 2015-03-15T16:08:09Z

Test build #28629 has started for PR 5029 at commit 5713d45.

This patch merges cleanly.

SparkQA · 2015-03-15T17:53:18Z

Test build #28629 has finished for PR 5029 at commit 5713d45.

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

AmplabJenkins · 2015-03-15T17:53:22Z

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

…in some tests, to further ensure they are cleaned up, and simplify

SparkQA · 2015-03-16T12:38:09Z

Test build #28647 has started for PR 5029 at commit 57609e4.

This patch merges cleanly.

SparkQA · 2015-03-16T14:25:32Z

Test build #28647 has finished for PR 5029 at commit 57609e4.

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

AmplabJenkins · 2015-03-16T14:25:36Z

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

sryza · 2015-03-16T22:26:44Z

What's the advantage of a parent directory created with createTempDir when we're already using File.createTempFile?

srowen · 2015-03-16T22:36:42Z

createTempDir registers it for deletion at shutdown.

sryza · 2015-03-16T22:50:13Z

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

@@ -370,7 +369,7 @@ class UtilsSuite extends FunSuite with ResetSystemProperties {
      assert(sparkConf.getBoolean("spark.test.fileNameLoadA", false) === true)
      assert(sparkConf.getInt("spark.test.fileNameLoadB", 1) === 2)
    } finally {
-      outFile.delete()
+      Utils.deleteRecursively(tmpDir)


Is this necessary now that we're using createTempDir?

This is redundant now.

sryza · 2015-03-16T22:53:14Z

It seems like in many places we're explicitly deleting the files in addition to using createTempDir. Is that necessary? Otherwise, LGTM.

vanzin · 2015-03-16T22:53:20Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/InsertIntoHiveTableSuite.scala

@@ -136,6 +135,7 @@ class InsertIntoHiveTableSuite extends QueryTest with BeforeAndAfter {
    assert(listFolders(tmpDir,List()).sortBy(_.toString()) == expected.sortBy(_.toString))
    sql("DROP TABLE table_with_partition")
    sql("DROP TABLE tmp_table")
+    Utils.deleteRecursively(tmpDir)


I'm torn on this, since it leaves temp files lying around while tests run, and, doesn't delete them if there's a hard crash. But the extra line of code is annoying everywhere and not done consistently. Hm.. Maybe the test superclass can manage a temp dir and delete it between tests. If anyone's interested in that, I'll go there. Otherwise maybe best to leave it as-is and not add recursive deletes where there were none before.

(A few of these changes are to make the temp file go in a temp dir instead of working dir, but most are indeed just about standardization and better guaranteeing cleanup)

I was going to suggest a "TempDirectory" trait or something to manage a temp dir per test. If scalatest runs a single JVM for all tests, then it might be worth it, but it would be a very noisy change.

srowen · 2015-03-16T22:53:22Z

True, there is not much consistency in whether it is also deleted explicitly after the test. I think I was mostly keeping that as it was in each case

vanzin · 2015-03-16T22:54:16Z

streaming/src/test/scala/org/apache/spark/streaming/ReceiverSuite.scala

@@ -293,6 +292,8 @@ class ReceiverSuite extends TestSuiteBase with Timeouts with Serializable {
      assert(sortedAllLogFiles1.takeRight(1).forall(leftLogFiles1.contains))
      assert(sortedAllLogFiles2.takeRight(3).forall(leftLogFiles2.contains))
    }
+
+    Utils.deleteRecursively(tempDirectory)


vanzin · 2015-03-16T22:56:26Z

LGTM (btw you're my hero).

srowen · 2015-03-17T11:41:10Z

OK, I basically left the code as-is with respect to recursive deletion. Yeah adding a new trait to every test might be too much for the moment. I think this zaps a few minor actual problems (stray temp files) and helps clean up more stuff in more cases, and that leaves us with little practical problem.

SparkQA · 2015-03-17T11:43:09Z

Test build #28718 has started for PR 5029 at commit 9004081.

This patch merges cleanly.

SparkQA · 2015-03-17T13:23:51Z

Test build #28718 has finished for PR 5029 at commit 9004081.

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

AmplabJenkins · 2015-03-17T13:23:55Z

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

marmbrus · 2015-03-18T03:23:12Z

SQL changes LGTM. I'll note that at the top of TestHive there is more code that calls getTempFilePath. I'm not actually sure if that is using the correct code path or not, but it looks like its trying to do its own cleanup.

srowen · 2015-03-18T13:51:06Z

Let me see if I can also refactor that bit of code in TestHive too, yeah.

SparkQA · 2015-03-19T15:13:11Z

Test build #28871 has started for PR 5029 at commit 4a212fa.

This patch merges cleanly.

SparkQA · 2015-03-19T16:54:56Z

Test build #28871 has finished for PR 5029 at commit 4a212fa.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-19T16:55:00Z

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

SparkQA · 2015-03-19T20:03:09Z

Test build #28884 has started for PR 5029 at commit 27b740a.

This patch merges cleanly.

SparkQA · 2015-03-19T21:46:00Z

Test build #28884 has finished for PR 5029 at commit 27b740a.

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

AmplabJenkins · 2015-03-19T21:46:04Z

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

Use Utils.createTempDir() to replace other temp file mechanisms used …

57609e4

…in some tests, to further ensure they are cleaned up, and simplify

srowen force-pushed the SPARK-6338 branch from 5713d45 to 57609e4 Compare March 16, 2015 12:34

sryza reviewed Mar 16, 2015
View reviewed changes

vanzin reviewed Mar 16, 2015
View reviewed changes

Revert some added recursive-delete calls

9004081

Standardize a bit more temp dir management

4a212fa

Fix hive-thriftserver tests that don't expect an existing dir

27b740a

asfgit closed this in 6f80c3e Mar 20, 2015

srowen deleted the SPARK-6338 branch March 20, 2015 15:09

liancheng mentioned this pull request Mar 26, 2015

[SPARK-6546][Build] Using the wrong code that will make spark compile failed!! #5198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPARK-6338 [CORE] Use standard temp dir mechanisms in tests to avoid orphaned temp files #5029

SPARK-6338 [CORE] Use standard temp dir mechanisms in tests to avoid orphaned temp files #5029

srowen commented Mar 14, 2015

SparkQA commented Mar 14, 2015

SparkQA commented Mar 14, 2015

AmplabJenkins commented Mar 14, 2015

SparkQA commented Mar 15, 2015

SparkQA commented Mar 15, 2015

AmplabJenkins commented Mar 15, 2015

SparkQA commented Mar 16, 2015

SparkQA commented Mar 16, 2015

AmplabJenkins commented Mar 16, 2015

sryza commented Mar 16, 2015

srowen commented Mar 16, 2015

sryza Mar 16, 2015

vanzin Mar 16, 2015

sryza commented Mar 16, 2015

vanzin Mar 16, 2015

srowen Mar 16, 2015

vanzin Mar 16, 2015

srowen commented Mar 16, 2015

vanzin Mar 16, 2015

vanzin commented Mar 16, 2015

srowen commented Mar 17, 2015

SparkQA commented Mar 17, 2015

SparkQA commented Mar 17, 2015

AmplabJenkins commented Mar 17, 2015

marmbrus commented Mar 18, 2015

srowen commented Mar 18, 2015

SparkQA commented Mar 19, 2015

SparkQA commented Mar 19, 2015

AmplabJenkins commented Mar 19, 2015

SparkQA commented Mar 19, 2015

SparkQA commented Mar 19, 2015

AmplabJenkins commented Mar 19, 2015

SPARK-6338 [CORE] Use standard temp dir mechanisms in tests to avoid orphaned temp files #5029

SPARK-6338 [CORE] Use standard temp dir mechanisms in tests to avoid orphaned temp files #5029

Conversation

srowen commented Mar 14, 2015

SparkQA commented Mar 14, 2015

SparkQA commented Mar 14, 2015

AmplabJenkins commented Mar 14, 2015

SparkQA commented Mar 15, 2015

SparkQA commented Mar 15, 2015

AmplabJenkins commented Mar 15, 2015

SparkQA commented Mar 16, 2015

SparkQA commented Mar 16, 2015

AmplabJenkins commented Mar 16, 2015

sryza commented Mar 16, 2015

srowen commented Mar 16, 2015

sryza Mar 16, 2015

Choose a reason for hiding this comment

vanzin Mar 16, 2015

Choose a reason for hiding this comment

sryza commented Mar 16, 2015

vanzin Mar 16, 2015

Choose a reason for hiding this comment

srowen Mar 16, 2015

Choose a reason for hiding this comment

vanzin Mar 16, 2015

Choose a reason for hiding this comment

srowen commented Mar 16, 2015

vanzin Mar 16, 2015

Choose a reason for hiding this comment

vanzin commented Mar 16, 2015

srowen commented Mar 17, 2015

SparkQA commented Mar 17, 2015

SparkQA commented Mar 17, 2015

AmplabJenkins commented Mar 17, 2015

marmbrus commented Mar 18, 2015

srowen commented Mar 18, 2015

SparkQA commented Mar 19, 2015

SparkQA commented Mar 19, 2015

AmplabJenkins commented Mar 19, 2015

SparkQA commented Mar 19, 2015

SparkQA commented Mar 19, 2015

AmplabJenkins commented Mar 19, 2015