You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by chenghao-intel <gi...@git.apache.org> on 2014/09/15 10:19:54 UTC
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
GitHub user chenghao-intel opened a pull request:
https://github.com/apache/spark/pull/2393
[SPARK-3529] [SQL] Delete the temp files after test exit
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chenghao-intel/spark delete_temp_on_exit
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2393.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2393
----
commit 4ecc9d49a83082806b9f713ee49565aecf5df764
Author: Cheng Hao <ha...@intel.com>
Date: 2014-09-12T01:58:51Z
Delete the temp files after test exit
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58462948
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21522/consoleFull) for PR 2393 at commit [`ba1797a`](https://github.com/apache/spark/commit/ba1797a34b252295c927e709657a74f3e89885b5).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by mattf <gi...@git.apache.org>.
Github user mattf commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-55781471
+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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58462878
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58951776
Thanks! Merged to master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58602197
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58601541
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58602694
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/334/consoleFull) for PR 2393 at commit [`3a6511f`](https://github.com/apache/spark/commit/3a6511ff2bee7b1cbbbda914c4fc4a7cbd9658a0).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-55563405
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20332/consoleFull) for PR 2393 at commit [`4ecc9d4`](https://github.com/apache/spark/commit/4ecc9d49a83082806b9f713ee49565aecf5df764).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58605205
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/334/consoleFull) for PR 2393 at commit [`3a6511f`](https://github.com/apache/spark/commit/3a6511ff2bee7b1cbbbda914c4fc4a7cbd9658a0).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58471062
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-55728033
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58463076
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/2393
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58601564
Also I merged #2670.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/2393#discussion_r17814084
--- Diff: 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() {
--- End diff --
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2393#discussion_r17533627
--- Diff: sql/hive/src/main/scala/org/apache/spark/sql/hive/TestHive.scala ---
@@ -41,7 +49,27 @@ import org.apache.spark.sql.SQLConf
import scala.collection.JavaConversions._
object TestHive
- 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() {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-56124852
Thank you all, I've removed the `Signal` and use the `Utils.deleteRecursively` instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-55969364
+1 for the `deleteOnExit`/`deleteRecursively` pattern.
@mattf According to its [Javadoc](http://docs.oracle.com/javase/7/docs/api/java/io/File.html#deleteOnExit()), `deleteOnExit` only works for normal JVM termination.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58465409
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21522/consoleFull) for PR 2393 at commit [`ba1797a`](https://github.com/apache/spark/commit/ba1797a34b252295c927e709657a74f3e89885b5).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-56569695
@chenghao-intel any thoughts on avoiding the use of InterruptUtils? I'd like to avoid relying on more Hive APIs that we have to.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-55694942
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`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58465412
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58475061
`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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58471362
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:
https://github.com/apache/spark/pull/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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58470589
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58470882
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp
files after...
Posted by Sean Owen <so...@cloudera.com>.
Hm deleteOnExit should at least not hurt and I thought it will delete dirs
if they are empty, which may be so if temp files inside never existed or
were cleaned up themselves. But yeah always delete explicitly in the normal
execution path even in the event of normal exceptions.
On Sep 19, 2014 3:00 AM, "mattf" <gi...@git.apache.org> wrote:
> Github user mattf commented on the pull request:
>
> https://github.com/apache/spark/pull/2393#issuecomment-56127248
>
> +1 lgtm
>
> fyi, i checked, deleteOnExit isn't an option because it cannot
> recursively delete
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
> For additional commands, e-mail: reviews-help@spark.apache.org
>
>
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by mattf <gi...@git.apache.org>.
Github user mattf commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-56127248
+1 lgtm
fyi, i checked, deleteOnExit isn't an option because it cannot recursively delete
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-55570548
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20332/consoleFull) for PR 2393 at commit [`4ecc9d4`](https://github.com/apache/spark/commit/4ecc9d49a83082806b9f713ee49565aecf5df764).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58463679
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3529] [SQL] Delete the temp files after...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2393#issuecomment-58471183
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org