moving BucketUtils.deleteOnExit -> IOUtils #5332

lbergelson · 2018-10-19T18:26:35Z

replacing existing deleteRecursivelyOnExit method with deleteOnExit
closes Unify BucketUtils.deleteRecursively()/deleteOnExit() with IOUtils.deleteRecursivelyOnExit() #5319

codecov-io · 2018-10-19T22:41:15Z

Codecov Report

Merging #5332 into master will decrease coverage by 0.005%.
The diff coverage is 73.418%.

@@               Coverage Diff               @@
##              master     #5332       +/-   ##
===============================================
- Coverage     87.083%   87.078%   -0.005%     
- Complexity     31232     31480      +248     
===============================================
  Files           1915      1930       +15     
  Lines         144130    145044      +914     
  Branches       15901     16074      +173     
===============================================
+ Hits          125513    126302      +789     
- Misses         12834     12890       +56     
- Partials        5783      5852       +69

Impacted Files	Coverage Δ	Complexity Δ
...nstitute/hellbender/utils/gcs/BucketUtilsTest.java	`59.406% <ø> (+3.103%)`	`12 <0> (-1)`	⬇️
...ls/genomicsdb/GenomicsDBImportIntegrationTest.java	`88.367% <0%> (ø)`	`80 <0> (ø)`	⬇️
...oadinstitute/hellbender/utils/gcs/BucketUtils.java	`78.231% <100%> (-1.647%)`	`39 <0> (-3)`
...rg/broadinstitute/hellbender/utils/io/IOUtils.java	`72.981% <100%> (+0.254%)`	`95 <4> (+2)`	⬆️
...institute/hellbender/utils/io/IOUtilsUnitTest.java	`79.205% <54.839%> (-2.526%)`	`46 <2> (+1)`
...nder/utils/io/DeleteRecursivelyOnExitPathHook.java	`75% <75%> (ø)`	`3 <3> (?)`
...tute/hellbender/utils/codecs/table/TableCodec.java	`77.778% <0%> (-4.365%)`	`14% <0%> (+3%)`
...itute/hellbender/tools/walkers/mutect/Mutect2.java	`78.846% <0%> (-3.376%)`	`25% <0%> (+1%)`
...r/tools/walkers/mutect/Mutect2IntegrationTest.java	`91.603% <0%> (-2.89%)`	`83% <0%> (+11%)`
...ools/walkers/mutect/SubsettedLikelihoodMatrix.java	`60.87% <0%> (-2.767%)`	`12% <0%> (+2%)`
... and 53 more

magicDGS · 2018-10-21T10:00:30Z

src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java

+     *
+     * @param fileToDelete file or directory to be deleted at JVM shutdown.
+     */
+    public static void deleteOnExit(final Path fileToDelete){


I added time ago a method in htsjdk which does a similar think (IOUtils.deleteOnExit), which has a common shutdown hook to delete all at the same time (DeleteOnExitPathHook) mimicking the behavior of the java.io.File#deleteOnExit.

I would suggest to add a common hook to delete recursively, and change the name of this method. Like that, it would not create too many threads to delete on exit.

@magicDGS Yeah, yours is better than this. I guess I should mimic what you did.

I've copied your class to do this now so it shouldn't create many threads.

I would also rename this method to deleteRecursivelyOnExit to be explicit - otherwise, the difference with the htsjdk version is not clear. Also, it would be nice to backport this to htsjdk at some point, and thus it will be easier to migrate afterwards.

* replacing existing deleteRecursivelyOnExit method with deleteOnExit * closes #5319

droazen

Back to @lbergelson with my comments

droazen · 2019-01-03T20:35:55Z

src/main/java/org/broadinstitute/hellbender/utils/gcs/BucketUtils.java

+            IOUtils.deleteOnExit(IOUtils.getPath(path + TabixUtils.STANDARD_INDEX_EXTENSION));
+            IOUtils.deleteOnExit(IOUtils.getPath(path + ".bai"));
+            IOUtils.deleteOnExit(IOUtils.getPath(path + ".md5"));
+            IOUtils.deleteOnExit(IOUtils.getPath(path.replaceAll(extension + "$", ".bai"))); //if path ends with extension, replace it with .bai


Should we add the fasta indices here too, now that we're in the business of writing fastas?

Should we change this to just create each file in it's own directory instead of trying to delete every possible index?

I'm going to punt on this one and do it in the follow up PR because it touches code in a few different places.

droazen · 2019-01-03T20:39:16Z

src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java

+     * @param rootPath is the file/directory to be deleted
+     */
+    public static void deleteRecursively(final Path rootPath) throws IOException {
+        final List<Path> pathsToDelete = Files.walk(rootPath)


Is Files.walk() guaranteed to succeed on a non-directory? I feel like when @nalinigans implemented the original recursive deletion method, she might have had a legitimate reason for avoiding doing recursive deletion on non-directories.

According to the doc the result of calling it on a single file is the file itself.

The {@code Stream} returned is guaranteed to have at least one
* element, the starting file itself.

droazen · 2019-01-03T20:40:41Z

src/main/java/org/broadinstitute/hellbender/utils/io/DeleteRecursivelyOnExitPathHook.java

+/**
+ * Class to hold a set of {@link Path} to be delete on the JVM exit through a shutdown hook.
+ *
+ * <p>This class is a modification of {@link htsjdk.samtools.util.nio}


Complete this @link

woops, done

droazen · 2019-01-03T20:42:20Z

src/main/java/org/broadinstitute/hellbender/utils/io/DeleteRecursivelyOnExitPathHook.java

+ *
+ * <p>This class is a modification of {@link htsjdk.samtools.util.nio}
+ */
+class DeleteRecursivelyOnExitPathHook {


Document the reason why this class is package-protected.

droazen · 2019-01-03T20:42:44Z

src/main/java/org/broadinstitute/hellbender/utils/io/DeleteRecursivelyOnExitPathHook.java

+    /**
+     * Adds a {@link Path} for deletion on JVM exit.
+     *
+     * @param path path to be deleted.


Add a note here that path can be a (non-empty) directory.

good call, done

droazen · 2019-01-03T20:50:27Z

src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java

@@ -695,15 +694,15 @@ public static Path createTempPath(String name, String extension) {
            }


This method seems redundant with BucketUtils.getTempFilePath(), which you've preserved.

Also should probably be unified with IOUtils.createTempFileInDirectory()/IOUtils.createTempFile() above, since those are the methods called by the widely-used BaseTest.createTempFile()

I refactored a ton of stuff in the next PR. Let's revisit this there instead. I may have done that already.

droazen · 2019-01-03T20:53:30Z

src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java

@@ -987,4 +969,27 @@ public static String getGenomicsDBPath(final String path) {
        }
        return genomicsdbPath;
    }
+
+    /**
+     * Schedule a file to be deleted on JVM shutdown.


file -> file or directory

deleted -> deleted recursively

And add explicit note that this calls IOUtils.deleteRecursively() on fileToDelete at shutdown

droazen · 2019-01-03T21:04:10Z

src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java

        File tempFile = createTempFile(String.format("testWriteThenReadFileIntoByteArray_%d_%d", fileSize, readBufferSize), "tmp");

        byte[] dataWritten = getDeterministicRandomData(fileSize);
        IOUtils.writeByteArrayToFile(dataWritten, tempFile);
        byte[] dataRead = IOUtils.readFileIntoByteArray(tempFile, readBufferSize);

        Assert.assertEquals(dataRead.length, dataWritten.length);
-        Assert.assertTrue(Arrays.equals(dataRead, dataWritten));
+        Assert.assertEquals(dataWritten, dataRead);


Order of args seems wrong here -- dataRead is the "actual" value here, isn't it?

yes, good catch

droazen · 2019-01-03T21:04:26Z

src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java

@@ -329,7 +330,7 @@ public void testWriteThenReadStreamIntoByteArray ( int fileSize, int readBufferS
        byte[] dataRead = IOUtils.readStreamIntoByteArray(new FileInputStream(tempFile), readBufferSize);

        Assert.assertEquals(dataRead.length, dataWritten.length);
-        Assert.assertTrue(Arrays.equals(dataRead, dataWritten));
+        Assert.assertEquals(dataWritten, dataRead);


Again, actual and expected values might be swapped here.

droazen · 2019-01-03T21:06:09Z

src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java

-
-        final File dir = new File(GATKBaseTest.publicTestDir + "I_SHOULD_HAVE_BEEN_DELETED");
-        IOUtils.deleteRecursivelyOnExit(dir);
+        final File dir = new File( "I_SHOULD_HAVE_BEEN_DELETED");


Do you really want to create this file in the working directory?

it's more visible that way.... but I can move it back to where it was...

lbergelson · 2019-01-04T19:09:30Z

@droazen Could you take a second pass on this when you get a chance?

droazen

👍 Latest version looks good @lbergelson. Still a bunch of refactoring to be done in this part of the codebase, but it can wait until the next PR. Merging!

droazen self-requested a review October 19, 2018 18:35

droazen self-assigned this Oct 19, 2018

lbergelson mentioned this pull request Oct 19, 2018

cleanup up BucketUtils #5334

Open

magicDGS reviewed Oct 21, 2018

View reviewed changes

lbergelson added 3 commits January 3, 2019 15:17

moving BucketUtils.deleteOnExit -> IOUtils

da70a4c

* replacing existing deleteRecursivelyOnExit method with deleteOnExit * closes #5319

mend

7b80a58

using magic-dgs style exit hook

bfd8eac

lbergelson force-pushed the lb_merge_delete_on_exits branch from 8ed010c to bfd8eac Compare January 3, 2019 20:26

droazen requested changes Jan 3, 2019

View reviewed changes

droazen assigned lbergelson and unassigned droazen Jan 3, 2019

responding to comments

79110eb

droazen approved these changes Jan 16, 2019

View reviewed changes

droazen merged commit 83c0b8c into master Jan 16, 2019

droazen deleted the lb_merge_delete_on_exits branch January 16, 2019 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

moving BucketUtils.deleteOnExit -> IOUtils #5332

moving BucketUtils.deleteOnExit -> IOUtils #5332

lbergelson commented Oct 19, 2018

codecov-io commented Oct 19, 2018 •

edited

magicDGS Oct 21, 2018

lbergelson Oct 22, 2018 •

edited

lbergelson Jan 3, 2019

magicDGS Jan 5, 2019

droazen left a comment

droazen Jan 3, 2019

lbergelson Jan 4, 2019

lbergelson Jan 4, 2019

droazen Jan 3, 2019

lbergelson Jan 3, 2019

droazen Jan 3, 2019

lbergelson Jan 3, 2019

droazen Jan 3, 2019

lbergelson Jan 3, 2019

droazen Jan 3, 2019

lbergelson Jan 3, 2019

droazen Jan 3, 2019

droazen Jan 3, 2019

lbergelson Jan 4, 2019

droazen Jan 3, 2019

droazen Jan 3, 2019

droazen Jan 3, 2019

lbergelson Jan 4, 2019

droazen Jan 3, 2019

lbergelson Jan 4, 2019

droazen Jan 3, 2019

lbergelson Jan 4, 2019

droazen Jan 3, 2019

lbergelson Jan 4, 2019

lbergelson commented Jan 4, 2019

droazen left a comment

		@@ -695,15 +694,15 @@ public static Path createTempPath(String name, String extension) {
		}

moving BucketUtils.deleteOnExit -> IOUtils #5332

moving BucketUtils.deleteOnExit -> IOUtils #5332

Conversation

lbergelson commented Oct 19, 2018

codecov-io commented Oct 19, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

lbergelson Oct 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson commented Jan 4, 2019

droazen left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 19, 2018 •

edited

lbergelson Oct 22, 2018 •

edited