SPARK-10721 Log warning when file deletion fails

[tedyu]

No description provided.

[srowen]

This really isn't an error ...

[tedyu]

Is the file going to be cleaned later ?

Do you have any suggestion w.r.t. what log level should be used ?

[srowen]

Error to me means something unrecoverable; the process can't continue. This is a warning maybe. It's maybe better to look across the code at places where File.delete() may fail silently and do something similar. And then open a JIRA for this small cross cutting change.

[tedyu]

I did some search prior to making the change:

$ find . -name '*.java' -exec grep 'file.delete(' {} ; -print
if (spill.file.exists() && !spill.file.delete()) {
./core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleExternalSorter.java
if (spill.file.exists() && ! spill.file.delete()) {
./core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriter.java
if (!file.delete()) {
./core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
if (!file.delete()) {
./core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java
boolean deleted = file.delete();
./network/common/src/main/java/org/apache/spark/network/util/JavaUtils.java

This file was the only one among the files where delete() is called but whose return value wasn't checked.

[srowen]

That grep assumes the File reference name ends in "file". I see 20-30 non-test usages of the method. It may not be worth logging a warning for every possible failure, but worth surveying the usages to see if any really ought to log something in case it doesn't delete.

[SparkQA]

Test build #42729 has finished for PR 8843 at commit 240aea1.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class Sort(

[SparkQA]

Test build #42733 has finished for PR 8843 at commit cd46f28.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class Sort(

[SparkQA]

Test build #42736 has finished for PR 8843 at commit 411a7c1.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class Sort(

[SparkQA]

Test build #42731 has finished for PR 8843 at commit 6abdb8a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class Sort(

[SparkQA]

Test build #42737 has finished for PR 8843 at commit e44577c.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class Sort(

[SparkQA]

Test build #42738 has finished for PR 8843 at commit 740de99.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #42740 has finished for PR 8843 at commit a9148c0.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class Sort(

[SparkQA]

Test build #42732 has finished for PR 8843 at commit 6962408.

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

[SparkQA]

Test build #42741 has finished for PR 8843 at commit 68edb55.

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

[jerryshao]

I think for most the places, Spark application shutdown hook will finally delete all temp files when application is exited, so temporary not deleted files will finally be cleaned. Besides, if this file is failed to deleted due to FS problem, it is better to throw out an exception to notify user.

[SparkQA]

Test build #42743 has finished for PR 8843 at commit 8fface7.

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

[jerryshao]

Checked again in some places like here, Spark also checked whether the file is successfully deleted and print an error log if not, though this file will finally be deleted when app is exited. So I'm wondering we need to have a consistent way to handle all this delete stuffs.

[tedyu]

What I have done in this PR is to keep consistency with existing code.

@srowen
Can you take another look ?

[srowen]

Yeah this looks good. These are cases where you might actually want to know if the file deletion fails. We'll have to see whether some of these turn out to be very noisy, but if so, we'd have to ask why the deletion is failing regularly and try to fix that. That is, I think these warnings would only trigger rarely and are therefore of some value.

[tedyu]

Let me know if there is more on my side that should be done.

[srowen]

Nit: make this static

[srowen]

You can use the {} placeholder syntax for slf4j loggers here, like in other Java files

[SparkQA]

Test build #42833 has finished for PR 8843 at commit 2b4bdac.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #42834 has finished for PR 8843 at commit 84e7467.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #42835 has finished for PR 8843 at commit 47640f6.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

A few of these are now missing the s prefix for interpolation.

[SparkQA]

Test build #42842 has finished for PR 8843 at commit dc06976.

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

[srowen]

One more tiny thing, since I know you'll need another pass to fix the two interpolations: the canonical order is private static final ...

[srowen]

Still missing interpolation here.

[SparkQA]

Test build #42853 has finished for PR 8843 at commit c059526.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #42851 has finished for PR 8843 at commit 2885c70.

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

[SparkQA]

Test build #42857 has finished for PR 8843 at commit 8561388.

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

[srowen]

Merged into master