Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
overloading BlockCompressedInputStream.checkTerminator to support NIO #890
Conversation
|
lbergelson |
Some checks were not successful
codecov/project
— Absolute coverage decreased by -0.003% but relative coverage increased by +13.527% compared to 630aa48
|
e14ebe5
|
droazen
was assigned
by lbergelson
Jun 6, 2017
lbergelson
requested a review
from droazen
Jun 6, 2017
codecov-io
commented
Jun 6, 2017
•
Codecov Report
@@ Coverage Diff @@
## master #890 +/- ##
===============================================
+ Coverage 65.045% 65.062% +0.017%
- Complexity 7239 7251 +12
===============================================
Files 528 528
Lines 31892 31942 +50
Branches 5444 5460 +16
===============================================
+ Hits 20744 20782 +38
- Misses 9008 9016 +8
- Partials 2140 2144 +4
|
| + return checkTermination(file == null ? null : file.toPath()); | ||
| + } | ||
| + | ||
| + public static FileTermination checkTermination(final Path path) throws IOException { |
droazen
Jun 7, 2017
Contributor
Expand existing unit tests to call into these two new overloads as well.
| + } | ||
| + } | ||
| + | ||
| + public static FileTermination checkTermination(SeekableByteChannel channel) throws IOException { |
droazen
Jun 7, 2017
Contributor
Add javadoc for the new overloads, including stream closing behavior.
| + return FileTermination.HAS_TERMINATOR_BLOCK; | ||
| + } | ||
| + final int bufsize = (int)Math.min(fileSize, BlockCompressedStreamConstants.MAX_COMPRESSED_BLOCK_SIZE); | ||
| + final byte[] buf = new byte[bufsize]; |
droazen
Jun 7, 2017
Contributor
While you're at it, you might want to rename buf as well to distinguish from the member variable
| - } else { | ||
| - return FileTermination.DEFECTIVE; | ||
| - } | ||
| + final ByteBuffer byteBuffer = ByteBuffer.wrap(buf, i + BlockCompressedStreamConstants.GZIP_BLOCK_PREAMBLE.length, 4); |
droazen
Jun 7, 2017
Contributor
You might want to change the naming here to distinguish from bytebuffer above
| + return FileTermination.DEFECTIVE; | ||
| + } | ||
| + | ||
| + private static void readFully(SeekableByteChannel channel, ByteBuffer dst) throws IOException { |
droazen
assigned lbergelson and unassigned droazen
Jun 7, 2017
|
|
lbergelson |
Some checks were not successful |
1734eb9
|
lbergelson
merged commit 98c2438
into
master
Jun 8, 2017
4 of 5 checks passed
lbergelson
deleted the
lb_block_compressed_input_stream_path branch
Jun 8, 2017
| public static FileTermination checkTermination(final File file) throws IOException { | ||
| - final long fileSize = file.length(); | ||
| + return checkTermination(file == null ? null : file.toPath()); |
pshapiro4broad
Jun 8, 2017
Won't this cause checkTermination(Path) to throw when file == null? If so avoiding the NPE here doesn't seem worthwhile. Maybe you want to return FileTermination.DEFECTIVE in that case?
lbergelson
Jun 8, 2017
Contributor
@pshapiro4broad It absolutely just shifts the npr to later. I wanted to just keep the exact behavior of the original function, which was to NPR somewhere later. It does probably makes sense to check for null and throw a nice exception instead of an NPR. Returning defective would be a bad idea for a null file I think since it would hide other errors.
| - raFile.close(); | ||
| + //if an exception was thrown we don't want to reset the position because that would be likely to throw again | ||
| + //and suppress the initial exception | ||
| + if(!exceptionThrown) { |
pshapiro4broad
Jun 8, 2017
This code is not consistent with its use of spaces around if elements. Here there's no space between if and (. On line 679 there is a space there, but not after the ). On line 654 there are spaces both before and after.
The code should be consistent; putting a space before and after the ()s is the java standard and is what most of the existing code does, so I would recommend formatting it that way.
The try on line 606 is also formatted inconsistently with respect to the surrounding code.
lbergelson
Jun 8, 2017
Contributor
That's a good point. I'm not sure it's worth opening another pr over though since I've already merged this one.
lbergelson commentedJun 6, 2017
BlockCompressedInputStream.checkTerminator only worked on files. Gatk4 needs it to accept Paths and ideally SeekableByteChannels directly. Adding overloads that take Path and SeekableByteChannel and refactoring the internals to use SeekableByteChannel instead of RandomAccessFile.
Checklist