Fix regression regarding pipeing #1124
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1124 +/- ##
===============================================
- Coverage 65.982% 65.978% -0.004%
Complexity 7653 7653
===============================================
Files 535 535
Lines 32489 32491 +2
Branches 5518 5518
===============================================
Hits 21437 21437
Misses 8909 8909
- Partials 2143 2145 +2
|
There was a problem hiding this comment.
@yfarjoun This seems ok to me, but it seems like a temporary solution. With the move towards Path everywhere, we'll probably still need to come back through and fix the underlying problem with SeekablePathStream, and probably reverse this vectoring, to just send of(File) to of(file.toPath).
| if (Files.isRegularFile(path) && Files.exists(path)) { | ||
| return new SamInputResource(new PathInputResource(path)); | ||
| } else { | ||
| return of(path.toFile()); |
There was a problem hiding this comment.
I had originally implemented it as
return of(new FileInputStream(path))
But that requires a try-catch block to guard against non-existant paths.
perhaps in the presence of the Files.exists(path) clause this is a non-issue and I can swallow the exception and return a null (which should never ever happen)?
happy to change it or not...as people prefer.
| } | ||
|
|
||
| @Test(singleThreaded = true, groups="unix") | ||
| public void testWriteAndReadFromPipe() throws IOException, InterruptedException, ExecutionException, TimeoutException { | ||
| @DataProvider() |
There was a problem hiding this comment.
I think turning this into a data provider is less clear that just having for x in Arrays.asList(true, false) in the test.
|
|
||
| if (Files.isRegularFile(path) && Files.exists(path)) { | ||
| return new SamInputResource(new PathInputResource(path)); | ||
| } else { |
There was a problem hiding this comment.
This needs a comment linking back to the discussion that spawned it. Otherwise it will cause much confusion in the future.
|
I added text, and fixed the indents. merging. |
| @@ -91,7 +92,12 @@ public static SamInputResource of(final File file) { | |||
|
|
|||
| /** Creates a {@link SamInputResource} reading from the provided resource, with no index. */ | |||
| public static SamInputResource of(final Path path) { | |||
| return new SamInputResource(new PathInputResource(path)); | |||
|
|
|||
| if (Files.isRegularFile(path) && Files.exists(path)) { | |||
There was a problem hiding this comment.
@yfarjoun Could you open a separate PR to remove the Files.exists() check here? This operation is expensive over certain NIO filesystems (eg., takes on the order of seconds over GCS), so forcing it at reader creation time could potentially add hours to the wall-clock time of a task that needed to access many bams over GCS.
It's also not particularly logical to go to the else clause here if the file doesn't exist -- if it's a non-existent GCS file, PathInputResource is better equipped to produce a more relevant error message.
Based on @tfenne 's branch #1118
includes: