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

NIO support for TMP_DIR #4469

Merged

Conversation

magicDGS
Copy link
Contributor

Extracted from #3998

@@ -358,7 +360,7 @@ private void generateDuplicateIndexes() {
final int maxInMemory = (int) Math.min((Runtime.getRuntime().maxMemory() * 0.25) / SortingLongCollection.SIZEOF,
(double) (Integer.MAX_VALUE - 5));
logger.info("Will retain up to " + maxInMemory + " duplicate indices before spilling to disk.");
this.duplicateIndexes = new SortingLongCollection(maxInMemory, TMP_DIR.toArray(new File[TMP_DIR.size()]));
this.duplicateIndexes = new SortingLongCollection(maxInMemory, (Path[]) TMP_DIR.stream().map(IOUtils::getPath).toArray());
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a cast, you can provide an argument to toArray(), e.g. TMP_DIR.stream().map(IOUtils::getPath).toArray(Path[]::new)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Argument(common=true, optional=true)
public List<File> TMP_DIR = new ArrayList<>();
@Argument(common=true, optional=true, doc = "List of temp directories to use.")
public List<String> TMP_DIR = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you kebabify this argument as part of this PR (--temp-dir)? Looks like we missed this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we just did a usage check on this argument, and it's ONLY used in MarkDuplicates. Given this, @lbergelson and I think that we should just remove this argument completely. Would you be ok with removing it @magicDGS, or are you currently depending on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I depend on it, but I can move it to the tools that require it.

Copy link
Member

Choose a reason for hiding this comment

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

@droazen Do we think a lot of users use this instead -Djava.io.tmpdir? Maybe it's worth keeping a shortcut too? Sorry to reconsider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was reconsidering it too (that's why I didn't add a commit), because maybe there are other codepaths using teh java property, which is set here, instead of directly using the parameter. Next commit will not have this fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the name was changed on StandardArgumentDefinitions.TMP_DIR_NAME after rebase.

logger.warn("Temp directory {}: unable to set permissions due to {}", p, e.getMessage());
}
// TODO - this should be p.toAbsolutePath().toUri().toString() to allow other FileSystems to be used (see above)
System.setProperty("java.io.tmpdir", p.toAbsolutePath().toString()); // in loop so that last one takes effect
Copy link
Contributor

Choose a reason for hiding this comment

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

@lbergelson suggests a special case here for file URIs to strip the file:// prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the next commit.

@droazen droazen self-requested a review March 12, 2018 19:43
@magicDGS magicDGS force-pushed the dgs_nio_support_for_tmp_dir branch from 91cf73c to 74bf120 Compare March 15, 2018 13:08
@magicDGS
Copy link
Contributor Author

Rebased to resolve conflicts and addressing comments.

// only set the permissions if they change
if (permissions.addAll(Arrays.asList(
PosixFilePermission.OWNER_READ, PosixFilePermission.GROUP_READ, PosixFilePermission.OTHERS_READ,
PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, this is related with #4513 - should probably resolve here.

@codecov-io
Copy link

codecov-io commented Mar 15, 2018

Codecov Report

Merging #4469 into master will decrease coverage by 0.104%.
The diff coverage is 79.221%.

@@               Coverage Diff               @@
##              master     #4469       +/-   ##
===============================================
- Coverage     86.657%   86.552%   -0.104%     
+ Complexity     29049     29026       -23     
===============================================
  Files           1808      1809        +1     
  Lines         134688    134751       +63     
  Branches       14938     14935        -3     
===============================================
- Hits          116716    116630       -86     
- Misses         12559     12714      +155     
+ Partials        5413      5407        -6
Impacted Files Coverage Δ Complexity Δ
...ellbender/cmdline/StandardArgumentDefinitions.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...va/org/broadinstitute/hellbender/GATKBaseTest.java 100% <ø> (ø) 6 <0> (ø) ⬇️
.../markduplicates/EstimateLibraryComplexityGATK.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...institute/hellbender/exceptions/UserException.java 69.63% <0%> (-1.047%) 3 <0> (ø)
...ols/walkers/markduplicates/MarkDuplicatesGATK.java 87.917% <100%> (ø) 69 <0> (ø) ⬇️
...kduplicates/MarkDuplicatesGATKIntegrationTest.java 97.826% <100%> (ø) 22 <0> (ø) ⬇️
...stitute/hellbender/cmdline/CommandLineProgram.java 84% <44.444%> (-1.906%) 43 <4> (-1)
...rg/broadinstitute/hellbender/utils/io/IOUtils.java 70.048% <71.429%> (+0.1%) 53 <2> (+2) ⬆️
...institute/hellbender/utils/io/IOUtilsUnitTest.java 87.719% <90.909%> (+0.471%) 32 <6> (+6) ⬆️
...der/cmdline/CommandLineProgramIntegrationTest.java 91.667% <91.667%> (ø) 5 <5> (?)
... and 10 more

@Argument(fullName = StandardArgumentDefinitions.TMP_DIR_NAME, common=true, optional=true)
public List<File> TMP_DIR = new ArrayList<>();
@Argument(fullName = StandardArgumentDefinitions.TMP_DIR_NAME, common=true, optional=true, doc = "List of temp directories to use.")
public List<String> TMP_DIR = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to keep this argument in GATK, can you make the type into a String instead of List<String>, since it's not doing anything intelligent right now with the List?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I modify that, it changes the behavior of MarkDuplicatesGATK and EstimateLibraryComplexityGATK (I guess that they are setting the temporary directory properly)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add a separate argument to those tools if they really need to take a List of temporary directories. But having the GATK-wide argument take a List and then ignore all but the last element is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Done.

if (permissions.addAll(Arrays.asList(
PosixFilePermission.OWNER_READ, PosixFilePermission.GROUP_READ, PosixFilePermission.OTHERS_READ,
PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE))) {
Files.setPosixFilePermissions(p, permissions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you get rid of this code to change the permissions on the temp dir, and throw a UserException instead if the temp dir is not writable/readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, although in that case is a change of behavior - if the temp dir is not used at all, this can fail for every tool...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we always use the temp dir in GATK -- if for nothing else, then to extract and load the GKL library for compression/decompression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the GKL library always extracted, even if not used at all? I don't think that it is (or should be) like that...

* @param path path to get the absolute name.
* @return a String with the absolute name.
*/
public static String getAbsolutePathName(final Path path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this method doesn't really tell you what it's doing -- can you rename to getAbsolutePathWithoutFileProtocol()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* the file:// prefix.
*
* @param path path to get the absolute name.
* @return a String with the absolute name.
Copy link
Contributor

Choose a reason for hiding this comment

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

"a String with the absolute name, and the file:// protocol removed, if it was present"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

try {
Files.createDirectories(p);
} catch (IOException e) {
// intentionally ignoring
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create the temp dir if it doesn't exist -- just throw a UserException in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally like specifying a non-existent directory for temporary files that is created by the program instead of explicitly using mkdirs - anyway, I can change in the next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in the next commit.

public void testGetAbsolutePathName(final String uriString, final String expected) {
Assert.assertEquals(IOUtils.getAbsolutePathName(IOUtils.getPath(uriString)), expected);
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a unit test to prove that the temp dir works over NIO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which kind of test do you need there? Retrieving the property and writing to it? Or running tools using it with NIO (e.g., MarkDuplicatesGATK)? Let me know for next round of comments...

Copy link
Contributor

Choose a reason for hiding this comment

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

How about a test that:

  1. Declares a dummy CommandLineProgram that executes createTempFile() and then asserts that it exists in its doWork() method

  2. Runs the above command line program with --tmp-dir set to a jimfs URI. You can create a jimfs like this:

        try (FileSystem jimfs = Jimfs.newFileSystem(Configuration.unix())) {
            final Path tmpDirPath = jimfs.getPath("tmp");
  1. You should also include a test case that runs the dummy CommandLineProgram with --tmp-dir set to a file:// URI, since that is a different code path.

You could put the test in a new CommandLineProgramIntegrationTest class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which createTempFile should be use in the test? There are several implementations (htsjdk, gatk, Files, File), and all of them have their own peculiarities. The ones that work with a File object won't work with this change. Thus, I am not sure how to proceed here - for the tools using the tmpDir this should work because the codebase uses the htsjdk Path implementation, but otherwise I am not sure how to get this done well...

On the other hand, there is a test class already for CLP (CommandLineProgramUnitTest) - should I still create the *IntegrationTest class or just use that one?

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

Second review complete, back to @magicDGS for changes

@magicDGS
Copy link
Contributor Author

Back to you, still without tests for tmp dir working with NIO due to lack of information: should it be at the tool level (e.g., running MarkDuplicatesGATK with NIO tmp dir - my main use case) or just checking the property value that is correctly set?

@magicDGS
Copy link
Contributor Author

@droazen - please, clarify how the tests should be done. I'm going to be on vacation next week, so feel free to add them to this branch if you would like to speed up the process (taking into account that #3998 depends on this now). Thanks!

@magicDGS
Copy link
Contributor Author

Friendly ping here @droazen

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

Added some more comments, back to you @magicDGS !

return new Object[][] {
{"/local/example.txt", "/local/example.txt"},
{"/local/file://example.txt", "/local/file:/example.txt"},
{"file:///local/example.txt", "/local/example.txt"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test case with a non-file URI (such as gcs://)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that you mean adding the gs:// example, that is the prefix in GCS. Otherwise it fails because IOUtils.getPath does not recognize it as a non-file URI.

Done and test passing.

if (permissions.addAll(Arrays.asList(
PosixFilePermission.OWNER_READ, PosixFilePermission.GROUP_READ, PosixFilePermission.OTHERS_READ,
PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE))) {
Files.setPosixFilePermissions(p, permissions);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we always use the temp dir in GATK -- if for nothing else, then to extract and load the GKL library for compression/decompression.

public void testGetAbsolutePathName(final String uriString, final String expected) {
Assert.assertEquals(IOUtils.getAbsolutePathName(IOUtils.getPath(uriString)), expected);
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

How about a test that:

  1. Declares a dummy CommandLineProgram that executes createTempFile() and then asserts that it exists in its doWork() method

  2. Runs the above command line program with --tmp-dir set to a jimfs URI. You can create a jimfs like this:

        try (FileSystem jimfs = Jimfs.newFileSystem(Configuration.unix())) {
            final Path tmpDirPath = jimfs.getPath("tmp");
  1. You should also include a test case that runs the dummy CommandLineProgram with --tmp-dir set to a file:// URI, since that is a different code path.

You could put the test in a new CommandLineProgramIntegrationTest class

if (this.TMP_DIR == null) this.TMP_DIR = new ArrayList<>();
if (this.TMP_DIR.isEmpty()) TMP_DIR.add(IOUtil.getDefaultTmpDir());
// TODO - this should use the HTSJDK IOUtil.getDefaultTmpDirPath, which is somehow broken in the current HTSJDK version
if (TMP_DIR == null || TMP_DIR.isEmpty()) TMP_DIR = IOUtils.getAbsolutePathWithoutFileProtocol(IOUtils.getPath(System.getProperty("java.io.tmpdir")));
Copy link
Contributor

Choose a reason for hiding this comment

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

Curly braces around if statement body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover from previous implementation. Changed as it is clearer.

@Argument(fullName = StandardArgumentDefinitions.TMP_DIR_NAME, common=true, optional=true)
public List<File> TMP_DIR = new ArrayList<>();
@Argument(fullName = StandardArgumentDefinitions.TMP_DIR_NAME, common=true, optional=true, doc = "Temp directory to use.")
public String TMP_DIR;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this variable to tmpDir (but keep the argument name itself as-is)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// TODO: it may be that the program does not need a tmp dir
// TODO: if it fails, the problem can be discovered downstream
// TODO: should log a warning instead?
throw new UserException.BadInput(String.format("--%s (%s) should exists and have read/write access",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use UserException.BadTmpDir instead (here and below)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, "should exists" should be "should exist"

Copy link
Member

Choose a reason for hiding this comment

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

@magicDGS You probably want to rebase before making this change since I just made some changes to BadTmpDir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using BadTmpDir as currently implemented is not really pointing out where the error is: if the property was not set, and fails on checking the access it will point to a different tmp directory. I added a new constructor for the exception with a Path to provide the one in use.

Done.

@magicDGS magicDGS force-pushed the dgs_nio_support_for_tmp_dir branch from 73a98f8 to 407c209 Compare May 2, 2018 11:05
@magicDGS
Copy link
Contributor Author

magicDGS commented May 2, 2018

@droazen - Still some questions about integration tests (on the comment with your suggestion, but here too):

  • I am not sure if the test that you are proposing will work with all the implementations of createTempFile: depends on how it is handle, as a File or as a Path
  • I think that this depends a lot on the parts of the codebase that we are looking at, so maybe before accepting this a pass should be done for the usages and how the java.io.tmpdir is used.

Waiting for your feedback before doing something that does not make sense...

@droazen
Copy link
Contributor

droazen commented May 3, 2018

@magicDGS Could you add overloads of IOUtils.createTempFile() and BaseTest.createTempFile() that take a Path, and use them just in the tests you write for this PR? Once tests are in place, we can gradually migrate usages to the Path overloads in future PRs.

@magicDGS magicDGS force-pushed the dgs_nio_support_for_tmp_dir branch from 407c209 to 280da96 Compare May 25, 2018 09:57
@magicDGS
Copy link
Contributor Author

Sorry for the delay here, @droazen - I had some technical problems with my hardware and did not have time to work on this. Back to you with implemented tests!

@droazen
Copy link
Contributor

droazen commented Jun 5, 2018

@magicDGS Could you rebase this branch one last time? Once it's been rebased, I'll give it a final look and (hopefully) hit merge. Thanks!

@droazen droazen assigned droazen and unassigned magicDGS Jun 5, 2018
@magicDGS magicDGS force-pushed the dgs_nio_support_for_tmp_dir branch from 280da96 to be00971 Compare June 6, 2018 15:21
@magicDGS
Copy link
Contributor Author

magicDGS commented Jun 6, 2018

Rebased @droazen - back to you!

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

@magicDGS This PR looks good now. I just had a few remaining comments on the test code, then we should be able to merge.

@@ -0,0 +1,98 @@
/*
* The MIT License (MIT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the license from this file. GATK4 has a repository-wide BSD 3-clause license.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed (sorry, my IDE attach sometimes old LICENSE config...)

/**
* @author Daniel Gomez-Sanchez (magicDGS)
*/
public class CommandLineProgramIntegrationTest extends GATKBaseTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Integration test classes should extend CommandLineProgramTest rather than GATKBaseTest directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
public class CommandLineProgramIntegrationTest extends GATKBaseTest {

private static FileSystem jimfs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Create the Jimfs filesystem within your test case rather than at the class level. Static variables in test code tend to cause problems down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a solution by creating a new method in GATKBaseTest to close Jimfs for a Path. I refactored to remove this static field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For whatever reason, that method fails - it looks like GATK's IOUtil.getPath does not recognize Jimfs if the instance is created in the test method. Again, no action here.

public Object[][] tmpDirs() throws Exception {
return new Object[][]{
{createTempDir("local").toPath()},
{Files.createDirectory(jimfs.getPath("tmp"))}
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be 3 test cases here rather than 2 in order to cover all the branches in your code: a local directory without a file:// scheme specified, a local directory with file:// specified, and a Jimfs URI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the one with the file:// prefix is already tested - in the test, the argument is converted with Path.toUri().toString(), which adds the file://. I refactored the method to has the String argument directly, and added a test case for a local file without prefix.

}

@Test(dataProvider = "tmpDirs", singleThreaded = true)
public void testCreateTempPath(final Path tmpDir) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be named something like testTmpDirArgument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

public Object[][] absoluteNames() {
return new Object[][] {
{"/local/example.txt", "/local/example.txt"},
{"/local/file://example.txt", "/local/file:/example.txt"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a strange one... can you explain why "/local/file://example.txt" becomes "/local/file:/example.txt"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to normalization of the path (removal of redundant /) - I added a comment here to clarify for the future.


final Path p = (Path) new TestCreateTempPathClp().instanceMain(new String[]{
"--" + StandardArgumentDefinitions.TMP_DIR_NAME, tmpDir.toUri().toString()});
Assert.assertTrue(Files.exists(p));
Copy link
Contributor

Choose a reason for hiding this comment

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

Also assert that the java.io.tmpdir property has been changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

*
* @return A file in the temporary directory starting with name, ending with extension, which will be deleted after the program exits.
*/
public static Path createTempPath(String name, String extension) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have a standalone unit test for this method in IOUtilsUnitTest (since the test in CommandLineProgramIntegrationTest is testing the --tmp-dir argument rather than this method directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -166,6 +166,21 @@ private void innerTestGetPath(String s) throws IOException {
Assert.assertTrue(size>0);
}

@DataProvider
public Object[][] absoluteNames() {
return new Object[][] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also include a test case with a non-absolute path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Assert.assertTrue(Files.exists(p));

// get back to the previous tmp dir
System.setProperty("java.io.tmpdir", previousTmpDir);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also put this in a finally clause to be sure that after the test it always resets the previous tmp directory

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

A small number of remaining comments on the tests. Back to @magicDGS.

try {
final Path p = (Path) new TestCreateTempPathClp().instanceMain(new String[] {
"--" + StandardArgumentDefinitions.TMP_DIR_NAME, tmpDirArg});
Assert.assertTrue(Files.exists(p));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also assert here that the parent directory of p is equal to the tmpDirArg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

final Path p = (Path) new TestCreateTempPathClp().instanceMain(new String[] {
"--" + StandardArgumentDefinitions.TMP_DIR_NAME, tmpDirArg});
Assert.assertTrue(Files.exists(p));
Assert.assertNotEquals(System.getProperty("java.io.tmpdir"), previousTmpDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally you would check here that java.io.tmpdir has been set to tmpDirArg, not just that it's different from previousTmpDir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/**
* Closes the {@link java.nio.file.FileSystem} for a path if it is an instance from {@link Jimfs}
*/
public static void closeFileSystemIfJimnfs(final Path path) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this method should be up in GATKBaseTest, since you reported that you weren't able to use it in CommandLineProgramIntegrationTest. You should use the same method to create the Jimfs instances across your different test classes. At this point I'd be satisfied if you just deleted this method, and used the @BeforeClass/@AfterClass approach to set up the Jimfs in IOUtilsUnitTest.

Copy link
Contributor Author

@magicDGS magicDGS Aug 3, 2018

Choose a reason for hiding this comment

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

Done

*/
public class CommandLineProgramIntegrationTest extends CommandLineProgramTest {

private static FileSystem jimfs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this non-static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@magicDGS
Copy link
Contributor Author

magicDGS commented Aug 3, 2018

@droazen - I also rebased locally to check the conflicting files: the differences are a new constant (DEFAULT_TOOLKIT_SHORT_NAME) and documentation for TMP_DIR (which is also added in this PR, but as it changed the List to String, it is a bit different). Do you want that I push the rebased branch, or do you prefer to wait until the end of the review?

@magicDGS
Copy link
Contributor Author

@droazen - friendly ping here!

@droazen
Copy link
Contributor

droazen commented Aug 24, 2018

@magicDGS It looks like all of my comments here have been addressed, but the branch is in conflict. If you do a last rebase I can (finally) hit merge!

@magicDGS magicDGS force-pushed the dgs_nio_support_for_tmp_dir branch from 28d90b5 to 0936dfb Compare August 24, 2018 17:47
@magicDGS
Copy link
Contributor Author

Rebased - waiting for test passing and hopefully you can hit merge @droazen!

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

👍 Looks good! Thanks for your patience across the many review iterations of this branch.

@droazen droazen merged commit 9125ff6 into broadinstitute:master Aug 24, 2018
@magicDGS
Copy link
Contributor Author

Thanks for the review @droazen!

@magicDGS magicDGS deleted the dgs_nio_support_for_tmp_dir branch August 27, 2018 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants