Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c0d5b28
Delete tmp dir when sc is stop
Sephiroth-Lin Feb 6, 2015
aeac518
Delete tmp dir when sc is stop
Sephiroth-Lin Feb 6, 2015
e2a2b1b
update comment
Sephiroth-Lin Feb 6, 2015
1d70926
update comment
Sephiroth-Lin Feb 6, 2015
d7ccc64
Change log level
Sephiroth-Lin Feb 7, 2015
b38e0f0
add dir check before delete
Sephiroth-Lin Feb 7, 2015
dd9686e
format code
Sephiroth-Lin Feb 7, 2015
f48a3c6
don't check sparkFilesDir, check executorId
Sephiroth-Lin Feb 7, 2015
b2018a5
check sparkFilesDir before delete
Sephiroth-Lin Feb 9, 2015
1339c96
add a member to store the reference of tmp dir
Sephiroth-Lin Feb 10, 2015
4edf394
rename the variable and update comment
Sephiroth-Lin Feb 10, 2015
b968e14
using an interpolated string
Sephiroth-Lin Feb 10, 2015
fbbc785
using an interpolated string
Sephiroth-Lin Feb 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions core/src/main/scala/org/apache/spark/HttpFileServer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ private[spark] class HttpFileServer(

def stop() {
httpServer.stop()

// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

def addFile(file: File) : String = {
Expand Down
8 changes: 8 additions & 0 deletions core/src/main/scala/org/apache/spark/SparkEnv.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ class SparkEnv (
// actorSystem.awaitTermination()

// Note that blockTransferService is stopped by BlockManager since it is started by it.

// 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(new File(sparkFilesDir))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this one is safe. Looking at the code, sparkFilesDir will be the current working directory on executors. Is that really safe to delete recursively?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree; this seems unsafe. It would be a disaster if we accidentally deleted directories that we didn't create, so we can't delete any path that could point to the CWD. Instead, we might be able to either ensure that the CWD is a subfolder of a spark local directory (so it will be cleaned up as part of our baseDir cleanup) or just change sparkFilesDir to not download files to the CWD (e.g. create a temporary directory in both the driver and executors).

Old versions of the addFile API contract said that files would be downloaded to the CWD, but we haven't made that promise since Spark 0.7-ish, I think; we only technically guarantee that SparkFIles.get will return the file paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srowen @JoshRosen you are right, as we only need to delete the tmp dir create by driver, so check executorId first to confirm only in driver we will delete the tmp dir. Thank you.

} catch {
case e: Exception => logError(s"Exception while deleting Spark temp dir: $sparkFilesDir", e)
}
}

private[spark]
Expand Down