/htsjdk

Obey the tmpDir setting in several constructors that currently ignore it #826

Merged
merged 2 commits into from May 2, 2017

Conversation

Reviewers

@droazen droazen

@yfarjoun yfarjoun

Assignees

@Lenbok Lenbok

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@Lenbok @codecov-io @cmnbroad @yfarjoun @droazen
@Lenbok
Contributor

Lenbok commented Mar 16, 2017 • edited

Description

Several constructors don't do what they should regarding tmpDir.

Ensured that both initWriter and initializeBAMWriter both call setTempDirectory() when needed, and made one of the existing constructors make use of initWriter() (This private method had an unused parameter, which has been removed)

We encountered this by running out of disk space while sorting a file during output.

Checklist

  • Code compiles correctly
  • New tests covering changes and new functionality
  • All tests passing
@codecov-io

codecov-io commented Mar 16, 2017 • edited

Codecov Report

Merging #826 into master will increase coverage by 0.031%.
The diff coverage is 66.667%.

@@               Coverage Diff               @@
##              master      #826       +/-   ##
===============================================
+ Coverage     64.916%   64.948%   +0.031%     
- Complexity      7200      7209        +9     
===============================================
  Files            527       527               
  Lines          31784     31784               
  Branches        5425      5424        -1     
===============================================
+ Hits           20633     20643       +10     
+ Misses          9016      9008        -8     
+ Partials        2135      2133        -2
Impacted Files Coverage Δ Complexity Δ
...c/main/java/htsjdk/samtools/SAMFileWriterImpl.java 74.026% <100%> (+6.026%) 24 <2> (+4) ⬆️
...ain/java/htsjdk/samtools/SAMFileWriterFactory.java 63.971% <57.143%> (+4.55%) 41 <1> (+4) ⬆️
...samtools/util/AsyncBlockCompressedInputStream.java 72% <0%> (-4%) 12% <0%> (-1%)
...main/java/htsjdk/samtools/SAMRecordSetBuilder.java 89.113% <0%> (+0.806%) 64% <0%> (+2%) ⬆️
@cmnbroad
Contributor

cmnbroad commented Mar 21, 2017 • edited

Hi @Lenbok - The fix looks good, but we'd like to have tests to verify these changes. Would you be able to add some getters on the writer for the properties such as tmpdir, maxRecordsInRam, etc., and then add some tests to verify that they're propagated from the factory to the writer instance for the affected code paths ? Thanks.
@Lenbok
Contributor

Lenbok commented Mar 24, 2017

@cmnbroad No problems, will do.
@Lenbok
Contributor

Lenbok commented Mar 27, 2017

@cmnbroad Added the accessors and augmented an existing helper method that is used for a couple of SAM and BAM writing tests to check the properties are passed into the writers. Also put a couple of getters on the factory as well.
@Lenbok
Contributor

Lenbok commented Apr 15, 2017

@cmnbroad status check?
src/test/java/htsjdk/samtools/SAMFileWriterFactoryTest.java
+ final File wontBeUsed = new File("wontBeUsed.tmp");
+ final int maxRecsInRam = 271828;
+ factory.setMaxRecordsInRam(maxRecsInRam);
+ factory.setTempDirectory(wontBeUsed);
@droazen

droazen Apr 18, 2017

Contributor

Can you make a separate test case to test this functionality instead of modifying an existing one?

@yfarjoun

yfarjoun Apr 18, 2017

Contributor

( you can copy the old test verbatim and put in your changes )

@yfarjoun

yfarjoun requested changes Apr 18, 2017
View changes

looks good to go except for the comment regarding the test case.

Lenbok was assigned by droazen Apr 18, 2017

Lenbok added some commits Mar 16, 2017

@Lenbok Lenbok Obey the tmpDir setting in several constructors that currently ignore… 
… it.

As part of this, made one constructor follow the existing convention of
calling initWriter() (This private method had an unused parameter, which
has been removed), and ensured that both initWriter and
initializeBAMWriter both call setTempDirectory() when needed.
 2702838
@Lenbok Lenbok Test that setMaxRecordsInRam and setTempDirectory settings on SAMFile… 
…WriterFactory make it to the writer implementation

All checks have passed

2 successful checks
codecov/project — 64.948% (+0.031%) compared to 78da175
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
19961c9
@Lenbok
Contributor

Lenbok commented Apr 18, 2017

@yfarjoun Have pulled out a separate test case and rebased to current master. Should be good to go now.
src/test/java/htsjdk/samtools/SAMFileWriterFactoryTest.java
+ Assert.assertEquals(maxRecsInRam, ((SAMFileWriterImpl) writer).getMaxRecordsInRam());
+ Assert.assertEquals(wontBeUsed, ((SAMFileWriterImpl) writer).getTempDirectory());
+ }
+ try (final SAMFileWriter writer = factory.makeSAMWriter(header, false, new ByteArrayOutputStream())) {
@yfarjoun

yfarjoun Apr 18, 2017

Contributor

is this code duplicated on purpose?

@Lenbok

Lenbok Apr 18, 2017

Contributor

Notice that one is testing SAM, and one is testing BAM. It didn't seem worth pulling out the two duplicated assertions to a separate method.

@yfarjoun

yfarjoun Apr 18, 2017

Contributor

ah. OK. sorry. I didn't see that.

@yfarjoun

yfarjoun approved these changes Apr 18, 2017
View changes

looks good.

@droazen? further comments?

src/test/java/htsjdk/samtools/SAMFileWriterFactoryTest.java
+ Assert.assertEquals(maxRecsInRam, ((SAMFileWriterImpl) writer).getMaxRecordsInRam());
+ Assert.assertEquals(wontBeUsed, ((SAMFileWriterImpl) writer).getTempDirectory());
+ }
+ try (final SAMFileWriter writer = factory.makeSAMWriter(header, false, new ByteArrayOutputStream())) {
@yfarjoun

yfarjoun Apr 18, 2017

Contributor

ah. OK. sorry. I didn't see that.

@yfarjoun
Contributor

yfarjoun commented May 2, 2017

👍

@yfarjoun yfarjoun merged commit dd78f77 into samtools:master May 2, 2017
2 checks passed

2 checks passed

codecov/project 64.948% (+0.031%) compared to 78da175
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment