[SPARK-5644] [Core]Delete tmp dir when sc is stop #4412

Sephiroth-Lin · 2015-02-06T01:42:37Z

When we run driver as a service, and for each time we run job we only call sc.stop, then will not delete tmp dir create by HttpFileServer and SparkEnv, it will be deleted until the service process exit, so we need to delete these tmp dirs when sc is stop directly.

When we run driver as a service, and for each time we run job we only call sc.stop, then will not delete tmp dir create by HttpFileServer and SparkEnv, it will be deleted until the service process exit

srowen · 2015-02-06T06:22:55Z

You'll need a JIRA for this. I think it's probably a harmless change, but, a process contains one SparkContext, and isn't generally doing anything else but Spark, so what problem does this cause in practice?

Sephiroth-Lin · 2015-02-06T06:52:36Z

@srowen we run a process as a service which will not stop. In this service process we will create SparkContext and run job and then stop it, because we only call sc.stop but not exit this service process so the tmp dirs created by HttpFileServer and SparkEnv will not be deleted after SparkContext is stopped, and this will lead to creating too many tmp dirs if we create many SparkContext to run job in this service process.

srowen · 2015-02-06T15:14:37Z

core/src/main/scala/org/apache/spark/HttpFileServer.scala

+    // If we only stop sc, but the driver process still run as a services then we need to delete 
+    // the tmp dir, if not, it will create too many tmp dirs
+    try {
+      Utils.deleteRecursively(baseDir)


This seems OK to me. I would make the error log a warning though as it's non-fatal. Consider using an interpolated string here as you do below.

make the error log a warning though as it's non-fatal.

sparkFilesDir is point to the current working dir in executor, and we only need to delete the tmp dir create by driver, for safe, we check it first.

srowen · 2015-02-07T13:34:57Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

+    // the tmp dir, if not, it will create too many tmp dirs.
+    // We only need to delete the tmp dir create by driver, because sparkFilesDir is point to the
+    // current working dir in executor which we do not need to delete.
+    if (SparkContext.DRIVER_IDENTIFIER == executorId) {


I see that this condition should indeed match the logic which controls whether this directory is a temp dir, but since the consequence of getting this wrong is pretty significant, I wonder if we should be more conservative. For example, save off the isDriver flag and use it in both places to be sure that these are exactly in sync? or maintain a separate reference to the temp dir that was created, which is None if no temp dir was created, and so you can be sure you can delete it if it's not None?

@srowen sorry, I don't very clear. You mean we can not use the executorId to distinguish the driver and executor?

I mean that this is not exactly the same condition that decided whether to make a temp dir, although, it looks like it will end up being the same. Since the consequence of accidentally getting this wrong or due to a later change, I suggest making this much more intimately bound. For example: save a reference to the temp dir if it exists. That you know you can delete.

@srowen Than you. If we want to make this much more intimately bound, may be we can check the sparkFilesDir directly, or else, we need to add a parameter to SparkEnv class.

I think you just need to store one more member, which is a reference to the tmp dir if it was created. See above. That's pretty much fool-proof and doesn't require any new params anywhere.

change from < if (SparkContext.DRIVER_IDENTIFIER == executorId) > to < if (sparkFilesDir != ".") >, and add comment where sparkFilesDir is created.

srowen · 2015-02-09T11:13:42Z

ok to test

SparkQA · 2015-02-09T12:38:19Z

Test build #27097 has finished for PR 4412 at commit b2018a5.

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

Sephiroth-Lin · 2015-02-10T07:31:19Z

@srowen thank you, now I add a member to store the reference of the tmp dir if it was created, please help to check again.

SparkQA · 2015-02-10T08:43:12Z

Test build #27186 has finished for PR 4412 at commit 1339c96.

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

srowen · 2015-02-10T09:34:16Z

core/src/main/scala/org/apache/spark/HttpFileServer.scala

+      Utils.deleteRecursively(baseDir)
+    } catch {
+      case e: Exception =>
+        logWarning("Exception while deleting Spark temp dir: " + baseDir.getAbsolutePath, e)


Nit: consider interpolation here

Sephiroth-Lin · 2015-02-10T11:06:44Z

@srowen thank you, please help to check again.

SparkQA · 2015-02-10T12:17:32Z

Test build #27199 has finished for PR 4412 at commit b968e14.

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

SparkQA · 2015-02-10T13:49:07Z

Test build #27200 has finished for PR 4412 at commit fbbc785.

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

srowen · 2015-02-10T17:06:05Z

OK, that's looking good to me. I'll pause shortly to let others review it too.

When we run driver as a service, and for each time we run job we only call sc.stop, then will not delete tmp dir create by HttpFileServer and SparkEnv, it will be deleted until the service process exit, so we need to delete these tmp dirs when sc is stop directly. Author: Sephiroth-Lin <[email protected]> Closes apache#4412 from Sephiroth-Lin/bug-fix-master-01 and squashes the following commits: fbbc785 [Sephiroth-Lin] using an interpolated string b968e14 [Sephiroth-Lin] using an interpolated string 4edf394 [Sephiroth-Lin] rename the variable and update comment 1339c96 [Sephiroth-Lin] add a member to store the reference of tmp dir b2018a5 [Sephiroth-Lin] check sparkFilesDir before delete f48a3c6 [Sephiroth-Lin] don't check sparkFilesDir, check executorId dd9686e [Sephiroth-Lin] format code b38e0f0 [Sephiroth-Lin] add dir check before delete d7ccc64 [Sephiroth-Lin] Change log level 1d70926 [Sephiroth-Lin] update comment e2a2b1b [Sephiroth-Lin] update comment aeac518 [Sephiroth-Lin] Delete tmp dir when sc is stop c0d5b28 [Sephiroth-Lin] Delete tmp dir when sc is stop Conflicts: core/src/main/scala/org/apache/spark/SparkEnv.scala

Sephiroth-Lin added 2 commits February 6, 2015 09:33

Delete tmp dir when sc is stop

c0d5b28

When we run driver as a service, and for each time we run job we only call sc.stop, then will not delete tmp dir create by HttpFileServer and SparkEnv, it will be deleted until the service process exit

Delete tmp dir when sc is stop

aeac518

When we run driver as a service, and for each time we run job we only call sc.stop, then will not delete tmp dir create by HttpFileServer and SparkEnv, it will be deleted until the service process exit

Sephiroth-Lin changed the title ~~Delete tmp dir when sc is stop~~ [SPARK-5644] Delete tmp dir when sc is stop Feb 6, 2015

Sephiroth-Lin added 2 commits February 6, 2015 15:15

update comment

e2a2b1b

update comment

1d70926

Sephiroth-Lin changed the title ~~[SPARK-5644] Delete tmp dir when sc is stop~~ [SPARK-5644] [Core]Delete tmp dir when sc is stop Feb 6, 2015

srowen reviewed Feb 6, 2015
View reviewed changes

Sephiroth-Lin added 4 commits February 7, 2015 09:45

Change log level

d7ccc64

make the error log a warning though as it's non-fatal.

add dir check before delete

b38e0f0

sparkFilesDir is point to the current working dir in executor, and we only need to delete the tmp dir create by driver, for safe, we check it first.

format code

dd9686e

don't check sparkFilesDir, check executorId

f48a3c6

srowen reviewed Feb 7, 2015
View reviewed changes

check sparkFilesDir before delete

b2018a5

change from < if (SparkContext.DRIVER_IDENTIFIER == executorId) > to < if (sparkFilesDir != ".") >, and add comment where sparkFilesDir is created.

add a member to store the reference of tmp dir

1339c96

srowen reviewed Feb 10, 2015
View reviewed changes

Sephiroth-Lin added 2 commits February 10, 2015 18:57

rename the variable and update comment

4edf394

using an interpolated string

b968e14

using an interpolated string

fbbc785

asfgit closed this in 52983d7 Feb 10, 2015

Sephiroth-Lin deleted the bug-fix-master-01 branch May 15, 2016 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-5644] [Core]Delete tmp dir when sc is stop #4412

[SPARK-5644] [Core]Delete tmp dir when sc is stop #4412

Sephiroth-Lin commented Feb 6, 2015

srowen commented Feb 6, 2015

Sephiroth-Lin commented Feb 6, 2015

srowen Feb 6, 2015

srowen Feb 7, 2015

Sephiroth-Lin Feb 9, 2015

srowen Feb 9, 2015

Sephiroth-Lin Feb 9, 2015

srowen Feb 9, 2015

srowen commented Feb 9, 2015

SparkQA commented Feb 9, 2015

Sephiroth-Lin commented Feb 10, 2015

SparkQA commented Feb 10, 2015

srowen Feb 10, 2015

Sephiroth-Lin Feb 10, 2015

Sephiroth-Lin commented Feb 10, 2015

SparkQA commented Feb 10, 2015

SparkQA commented Feb 10, 2015

srowen commented Feb 10, 2015

[SPARK-5644] [Core]Delete tmp dir when sc is stop #4412

[SPARK-5644] [Core]Delete tmp dir when sc is stop #4412

Conversation

Sephiroth-Lin commented Feb 6, 2015

srowen commented Feb 6, 2015

Sephiroth-Lin commented Feb 6, 2015

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

srowen commented Feb 9, 2015

SparkQA commented Feb 9, 2015

Sephiroth-Lin commented Feb 10, 2015

SparkQA commented Feb 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sephiroth-Lin commented Feb 10, 2015

SparkQA commented Feb 10, 2015

SparkQA commented Feb 10, 2015

srowen commented Feb 10, 2015