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.

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-11974][CORE]Not all the temp dirs had been deleted when the JVM exits #9951

Closed
wants to merge 4 commits into from
Closed

Conversation

pzzs
Copy link
Contributor

@pzzs pzzs commented Nov 25, 2015

deleting the temp dir like that


scala> import scala.collection.mutable
import scala.collection.mutable

scala> val a = mutable.Set(1,2,3,4,7,0,8,98,9)
a: scala.collection.mutable.Set[Int] = Set(0, 9, 1, 2, 3, 7, 4, 8, 98)

scala> a.foreach(x => {a.remove(x) })

scala> a.foreach(println(_))
98

You may not modify a collection while traversing or iterating over it.This can not delete all element of the collection

@rxin
Copy link
Contributor

rxin commented Nov 25, 2015

What's the problem with the original approach?

@pzzs
Copy link
Contributor Author

pzzs commented Nov 25, 2015

It can not delete all element of shutdownDeletePaths.
Like the example above, this method can not delete all element of a.
@rxin

@rxin
Copy link
Contributor

rxin commented Nov 25, 2015

The part I don't get is --- where are we deleting elements of shutdownDeletePaths?

@pzzs
Copy link
Contributor Author

pzzs commented Nov 25, 2015

shutdownDeletePaths.foreach { dirPath =>
      try {
        logInfo("Deleting directory " + dirPath)
        Utils.deleteRecursively(new File(dirPath))
      } catch {
        case e: Exception => logError(s"Exception while deleting Spark temp dir: $dirPath", e)
      }
    }

Utils.deleteRecursively(new File(dirPath) call ShutdownHookManager.removeShutdownDeleteDir(file)

def deleteRecursively(file: File) {
   ...
       ShutdownHookManager.removeShutdownDeleteDir(file)
   ...
  }

ShutdownHookManager.removeShutdownDeleteDir(file)will deleting elements of shutdownDeletePaths

  def removeShutdownDeleteDir(file: File) {
    val absolutePath = file.getAbsolutePath()
    shutdownDeletePaths.synchronized {
      shutdownDeletePaths.remove(absolutePath)
    }
  }

@rxin

@rxin
Copy link
Contributor

rxin commented Nov 25, 2015

Ah ok - can you update the change to add a line of comment saying we need to materialize the paths to delete because deleteRecursively removes items from shutdownDeletePaths as we are traversing through it?

 //we need to materialize the paths to delete because deleteRecursively removes items from
 //shutdownDeletePaths as we are traversing through it.
@pzzs
Copy link
Contributor Author

pzzs commented Nov 25, 2015

Ok, get it @rxin

@@ -57,7 +57,9 @@ private[spark] object ShutdownHookManager extends Logging {
// Add a shutdown hook to delete the temp dirs when the JVM exits
addShutdownHook(TEMP_DIR_SHUTDOWN_PRIORITY) { () =>
logInfo("Shutdown hook called")
shutdownDeletePaths.foreach { dirPath =>
//we need to materialize the paths to delete because deleteRecursively removes items from
Copy link
Contributor

Choose a reason for hiding this comment

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

need a space after // - otherwise this will fail the style check

@pzzs
Copy link
Contributor Author

pzzs commented Nov 25, 2015

Is it OK? @rxin

@SparkQA
Copy link

SparkQA commented Nov 25, 2015

Test build #2108 has finished for PR 9951 at commit a3e7cb8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * public final class SortedIterator extends UnsafeSorterIterator\n * case class GetStructField(child: Expression, ordinal: Int, name: Option[String] = None)\n * case class PrettyAttribute(name: String, dataType: DataType = NullType)\n

@rxin
Copy link
Contributor

rxin commented Nov 25, 2015

I've merged this. Thanks.

asfgit pushed a commit that referenced this pull request Nov 25, 2015
…VM exits

deleting the temp dir like that

```

scala> import scala.collection.mutable
import scala.collection.mutable

scala> val a = mutable.Set(1,2,3,4,7,0,8,98,9)
a: scala.collection.mutable.Set[Int] = Set(0, 9, 1, 2, 3, 7, 4, 8, 98)

scala> a.foreach(x => {a.remove(x) })

scala> a.foreach(println(_))
98
```

You may not modify a collection while traversing or iterating over it.This can not delete all element of the collection

Author: Zhongshuai Pei <peizhongshuai@huawei.com>

Closes #9951 from DoingDone9/Bug_RemainDir.

(cherry picked from commit 6b78157)
Signed-off-by: Reynold Xin <rxin@databricks.com>
asfgit pushed a commit that referenced this pull request Nov 25, 2015
…VM exits

deleting the temp dir like that

```

scala> import scala.collection.mutable
import scala.collection.mutable

scala> val a = mutable.Set(1,2,3,4,7,0,8,98,9)
a: scala.collection.mutable.Set[Int] = Set(0, 9, 1, 2, 3, 7, 4, 8, 98)

scala> a.foreach(x => {a.remove(x) })

scala> a.foreach(println(_))
98
```

You may not modify a collection while traversing or iterating over it.This can not delete all element of the collection

Author: Zhongshuai Pei <peizhongshuai@huawei.com>

Closes #9951 from DoingDone9/Bug_RemainDir.

(cherry picked from commit 6b78157)
Signed-off-by: Reynold Xin <rxin@databricks.com>
asfgit pushed a commit that referenced this pull request Nov 25, 2015
…VM exits

deleting the temp dir like that

```

scala> import scala.collection.mutable
import scala.collection.mutable

scala> val a = mutable.Set(1,2,3,4,7,0,8,98,9)
a: scala.collection.mutable.Set[Int] = Set(0, 9, 1, 2, 3, 7, 4, 8, 98)

scala> a.foreach(x => {a.remove(x) })

scala> a.foreach(println(_))
98
```

You may not modify a collection while traversing or iterating over it.This can not delete all element of the collection

Author: Zhongshuai Pei <peizhongshuai@huawei.com>

Closes #9951 from DoingDone9/Bug_RemainDir.

(cherry picked from commit 6b78157)
Signed-off-by: Reynold Xin <rxin@databricks.com>
@asfgit asfgit closed this in 6b78157 Nov 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants