[SPARK-3529] [SQL] Delete the temp files after test exit #2393

chenghao-intel · 2014-09-15T08:19:50Z

There are lots of temporal files created by TestHive under the /tmp by default, which may cause potential performance issue for testing. This PR will automatically delete them after test exit.

SparkQA · 2014-09-15T08:24:13Z

QA tests have started for PR 2393 at commit 4ecc9d4.

This patch merges cleanly.

SparkQA · 2014-09-15T09:51:03Z

QA tests have finished for PR 2393 at commit 4ecc9d4.

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

srowen · 2014-09-15T09:57:20Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/TestHive.scala

-  extends TestHiveContext(new SparkContext("local[2]", "TestSQLContext", new SparkConf()))
+  extends TestHiveContext(new SparkContext("local[2]", "TestSQLContext", new SparkConf())) {
+
+  Signal.handle(new Signal("INT"), new SignalHandler() {


Yikes, this seems a whole lot more heavy handed than just implementing test lifecycle methods with annotations. Elsewhere in the test framework, temp files are reliably delete by:

Invoking the standard method to get a temp dir

... which calls deleteOnExit()

... which also cleans up the declared test dir in an annotated cleanup method

I would really avoid use of Signal! It does not seem required and is inconsistent with other tests.

chenghao-intel · 2014-09-16T04:47:10Z

Thank you @srowen I think you're right, we should provide a mechanism to delete all of temp files in the test framework, not just for SQL on exit. I will investigate that.

Signal here is for manually kill the test by Ctrl^c, sorry, I am not sure your mean inconsistent with other tests

srowen · 2014-09-16T11:16:32Z

@chenghao-intel For example, in FileServerSuite:

  override def beforeAll() {
    super.beforeAll()
    tmpDir = Files.createTempDir()
    tmpDir.deleteOnExit()
    ...
  }
  ...

  override def afterAll() {
    super.afterAll()
    Utils.deleteRecursively(tmpDir)
  }

This makes a temp dir in the right place, asks the JVM to delete it on shutdown, but also tries to delete it explicitly after the test finishes.

(Looks like FileUtils.deleteDirectory duplicates Utils.deleteRecursively?)

I was not sure if deleteOnExit() works in the case of Ctrl-C, but a quick test here suggests it does. It won't work in the case of hard failures, but nothing will really.

The risk with Signal is that by trapping Ctrl-C (and this requires a non-public Sun API anyway) you become unkillable if the handler blocks for some reason.

mattf · 2014-09-16T17:36:54Z

+1 @srowen, using signal is too heavy handed

i'm skeptical that the jvm can guarantee dir removal on failure (say kill -9 or a jvm segv). those cases are hopefully less frequent than the current leaking of tmp files.

liancheng · 2014-09-17T22:21:26Z

+1 for the deleteOnExit/deleteRecursively pattern.

@mattf According to its Javadoc, deleteOnExit only works for normal JVM termination.

chenghao-intel · 2014-09-19T01:14:40Z

Thank you all, I've removed the Signal and use the Utils.deleteRecursively instead.

mattf · 2014-09-19T02:00:41Z

+1 lgtm

fyi, i checked, deleteOnExit isn't an option because it cannot recursively delete

marmbrus · 2014-09-19T23:01:15Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/TestHive.scala

@@ -69,11 +82,22 @@ class TestHiveContext(sc: SparkContext) extends HiveContext(sc) {
    setConf("javax.jdo.option.ConnectionURL",
      s"jdbc:derby:;databaseName=$metastorePath;create=true")
    setConf("hive.metastore.warehouse.dir", warehousePath)
+    HiveInterruptUtils.add(new HiveInterruptCallback() {


Is this really what InterruptUtils is for? It looks likes its for interrupting a running query. Why aren't these just part of the shutdown hook?

marmbrus · 2014-09-23T18:46:49Z

@chenghao-intel any thoughts on avoiding the use of InterruptUtils? I'd like to avoid relying on more Hive APIs that we have to.

chenghao-intel · 2014-10-09T04:48:06Z

Sorry for late reply. Thank you @marmbrus , it was quite error-pone by using the HiveInterruptUtils. I've updated the code with our own callback function.

SparkQA · 2014-10-09T04:49:40Z

QA tests have started for PR 2393 at commit ba1797a.

This patch merges cleanly.

AmplabJenkins · 2014-10-09T04:52:20Z

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

srowen · 2014-10-09T05:05:32Z

Sorry for the late reply as well, but -1 since this introduces again Commons IO FileUtils which broke the build recently. Use Utils.deleteRecursively.

Why a new callback trait instead of just a Runnable or Callable?
Also, Utils already has a mechanism for deleting files at shutdown from temp dirs. It would be good to avoid another shutdown hook.

SparkQA · 2014-10-09T05:37:36Z

QA tests have finished for PR 2393 at commit ba1797a.

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

AmplabJenkins · 2014-10-09T05:37:38Z

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

chenghao-intel · 2014-10-09T07:00:41Z

@srowen thank you very much, this is quite informative.
I've updated the code with Utils.registerShutdownDeleteDir, the code is very clean now! However, the temp directory still there when test exit, seems the register doesn't work on purpose, should I file another jira issue for this?

srowen · 2014-10-09T07:04:57Z

Perhaps you can debug a bit first to see if the shutdown hook is called and it attempts to delete the dir? is there are error while deleting it? this mechanism appears to work for unit test temp files.

AmplabJenkins · 2014-10-09T07:07:19Z

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

chenghao-intel · 2014-10-09T07:08:57Z

Actually, I searched the code, seems we haven't set the shutdown hook for Utils, you mean I have to do that myself ? I was thinking it should be set properly somewhere in spark core.

srowen · 2014-10-09T07:11:27Z

Ah, right. This is the shutdown hook:
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/util/Utils.scala#L257

... but it requires you to have used createTempDir(). Is this a temp dir?

I have an outstanding PR that improves a few things here, including, just directly deleting everything seen by registerShutdownDeleteDir in a single shutdown hook:
#2670

If that PR looks OK then, it would be good to get it committed, since it would make @chenghao-intel 's simpler solution here work.

chenghao-intel · 2014-10-09T07:57:27Z

TestHive creates temporal files/directories by specifying the prefix / suffix, it will be great if Utils support that as well, the code will be even more cleaner then. Currently I prefer wait for the #2670 to be merged.

marmbrus · 2014-10-10T01:23:26Z

I'm not super tied to the temp dirs with prefix/suffix (though that is kind of nice). I'm find changing that code to use whatever standard mechanisms the rest of spark uses.

marmbrus · 2014-10-10T01:23:45Z

Also I merged #2670.

chenghao-intel · 2014-10-10T01:33:06Z

Thank you @marmbrus , you're right!
I've just rebased the latest master, and the temp directory will be deleted after unit test exit locally.

So, I think this is ready to be merged too.

SparkQA · 2014-10-10T01:41:49Z

QA tests have started for PR 2393 at commit 3a6511f.

This patch merges cleanly.

SparkQA · 2014-10-10T02:27:21Z

QA tests have finished for PR 2393 at commit 3a6511f.

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

marmbrus · 2014-10-13T20:42:30Z

Thanks! Merged to master.

srowen reviewed Sep 15, 2014
View reviewed changes

marmbrus reviewed Sep 19, 2014
View reviewed changes

chenghao-intel force-pushed the delete_temp_on_exit branch from e790423 to 63cffd5 Compare October 9, 2014 04:41

Remove the temp dir after text exit

3a6511f

chenghao-intel force-pushed the delete_temp_on_exit branch from ba1797a to 3a6511f Compare October 9, 2014 06:56

asfgit closed this in d3cdf91 Oct 13, 2014

chenghao-intel deleted the delete_temp_on_exit branch December 10, 2014 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-3529] [SQL] Delete the temp files after test exit #2393

[SPARK-3529] [SQL] Delete the temp files after test exit #2393

chenghao-intel commented Sep 15, 2014

SparkQA commented Sep 15, 2014

SparkQA commented Sep 15, 2014

srowen Sep 15, 2014

chenghao-intel commented Sep 16, 2014

srowen commented Sep 16, 2014

mattf commented Sep 16, 2014

liancheng commented Sep 17, 2014

chenghao-intel commented Sep 19, 2014

mattf commented Sep 19, 2014

marmbrus Sep 19, 2014

marmbrus commented Sep 23, 2014

chenghao-intel commented Oct 9, 2014

SparkQA commented Oct 9, 2014

AmplabJenkins commented Oct 9, 2014

srowen commented Oct 9, 2014

SparkQA commented Oct 9, 2014

AmplabJenkins commented Oct 9, 2014

chenghao-intel commented Oct 9, 2014

srowen commented Oct 9, 2014

AmplabJenkins commented Oct 9, 2014

chenghao-intel commented Oct 9, 2014

srowen commented Oct 9, 2014

chenghao-intel commented Oct 9, 2014

marmbrus commented Oct 10, 2014

marmbrus commented Oct 10, 2014

chenghao-intel commented Oct 10, 2014

SparkQA commented Oct 10, 2014

SparkQA commented Oct 10, 2014

marmbrus commented Oct 13, 2014

[SPARK-3529] [SQL] Delete the temp files after test exit #2393

[SPARK-3529] [SQL] Delete the temp files after test exit #2393

Conversation

chenghao-intel commented Sep 15, 2014

SparkQA commented Sep 15, 2014

SparkQA commented Sep 15, 2014

srowen Sep 15, 2014

Choose a reason for hiding this comment

chenghao-intel commented Sep 16, 2014

srowen commented Sep 16, 2014

mattf commented Sep 16, 2014

liancheng commented Sep 17, 2014

chenghao-intel commented Sep 19, 2014

mattf commented Sep 19, 2014

marmbrus Sep 19, 2014

Choose a reason for hiding this comment

marmbrus commented Sep 23, 2014

chenghao-intel commented Oct 9, 2014

SparkQA commented Oct 9, 2014

AmplabJenkins commented Oct 9, 2014

srowen commented Oct 9, 2014

SparkQA commented Oct 9, 2014

AmplabJenkins commented Oct 9, 2014

chenghao-intel commented Oct 9, 2014

srowen commented Oct 9, 2014

AmplabJenkins commented Oct 9, 2014

chenghao-intel commented Oct 9, 2014

srowen commented Oct 9, 2014

chenghao-intel commented Oct 9, 2014

marmbrus commented Oct 10, 2014

marmbrus commented Oct 10, 2014

chenghao-intel commented Oct 10, 2014

SparkQA commented Oct 10, 2014

SparkQA commented Oct 10, 2014

marmbrus commented Oct 13, 2014