Resiliency: Add logging/safety when deleting files

[rmuir]

File.delete() is not great as it just returns a boolean if it succeeds or fails.

Instead we can use Files.delete/Files.deleteIfExists, which throws a real exception when it fails (e.g. DirectoryNotEmptyException, Access Denied, etc)

Brings in helpers from lucene (temporarily as XIOUtils):

• deleteFilesIfExist(File...): removes all the files, throwing the first exception with suppressions for other exceptions, but always attempts to remove all files
• deleteFilesIgnoringExceptions(File...): removes all files, suppressing all exceptions. good for when handling other exceptions
• rm(File...): recursive removal of all files, on any failure throws exception that contains each file name and the exception it hit.

Bans File.delete() with forbidden APIs.

This is also an opportunity to review all places in the codebase where we delete files, since they are all touched in this PR.

[rmuir]

Some possible bugs i might have made to look out for:

• Not being picky enough in tests (e.g. suppressing an exception when we should fail the test because it will just cause another confusing test failure if we can't delete the file)
• Calling Files.delete when it should be Files.deleteIfExists in source code (could cause excessive logging)

Additionally, since we have the chance now, we may want to also add any missing logging before we try to delete any file, so when debugging you can see all file deletes (in the successful case, too)

[rjernst]

was this unused?

[rjernst]

LGTM!

[s1monw]

instead of all these try catch blocks can we have a utility that accepts a logger?

[rmuir]

we could, but I worry this would mostly defeat the benefit of this patch.

One main reason Files.delete is better than File.delete, because instead of returning a boolean value on failure (which is easily ignored), it forces you to think about what should happen on IOException, since its a checked exception.

If we make an "easy-to-use" utility method that "shoves it under the rug", we gain nothing.
So I think call sites should deal with this on a case-by-case basis.

Deleting files is serious business: I don't think we need a lot of abstractions and ease-of-use around it?

[rmuir]

By the way, an alternative, that might make us both happy, would be to ban all other deletion methods, and add a utility method as you suggest that always logs any errors but still throws them.

This would enforce that both all file deletions are logged, and still enforce that callers have to deal with "what to do when things go wrong".

The cost would be some more complexity though.

[s1monw]

I left one comment other than that LGTM

[s1monw]

this has been fixed by #8366