[SPARK-3970] Remove duplicate removal of local dirs #2826
Conversation
viirya
changed the title
| @@ -140,7 +140,6 @@ private[spark] class DiskBlockManager(blockManager: BlockManager, conf: SparkCon | |||
| } | |||
|
|
|||
| private def addShutdownHook() { | |||
| localDirs.foreach(localDir => Utils.registerShutdownDeleteDir(localDir)) | |||
There was a problem hiding this comment.
It seems undesirable to duplicate the delete logic but I can see that stop needs to trigger a delete. What exception happens ? Deleting something that isn't there should not be an erro .
There was a problem hiding this comment.
Because it tries to delete local dirs that are already removed, So it would cause exceptions as following:
ERROR DiskBlockManager: Exception while deleting local spark dir: /tmp/spark-local-20141016160713-d38f
java.io.IOException: Failed to list files for dir: /tmp/spark-local-20141016160713-d38f/1b
at org.apache.spark.util.Utils$.listFilesSafely(Utils.scala:666)
at org.apache.spark.util.Utils$.deleteRecursively(Utils.scala:680)
.....
There was a problem hiding this comment.
Ah right. I really think deleteRecursively just shouldn't fail in this situation. If the dir can't be listed it should move on. Unfortunately null means "not a dir, or, an error occurred". So it may be resolvable by checking if the dir exists to begin with.
There was a problem hiding this comment.
Adding a check to see if the dir exits in listFilesSafely helps. I add another commit for that. However, the duplicate delete logic is undesirable and should not be there too.
|
ok to test |
|
QA tests have started for PR 2826 at commit
|
|
QA tests have finished for PR 2826 at commit
|
|
LGTM, other comments @srowen? |
|
Is this patch ok to merge? |
|
retest this please, just because it's been a while since jenkins last ran. |
|
Test build #22249 timed out for PR 2826 at commit |
|
retest this please |
|
Test build #22258 has finished for PR 2826 at commit
|
|
Ok I'm merging this |
The shutdown hook of
DiskBlockManagerwould remove localDirs. So do not need to register them withUtils.registerShutdownDeleteDir. It causes duplicate removal of these local dirs and corresponding exceptions.