Htsget POST request support #1529
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1529 +/- ##
===============================================
+ Coverage 69.417% 69.477% +0.060%
- Complexity 8924 8981 +57
===============================================
Files 602 606 +4
Lines 35520 35770 +250
Branches 5904 5944 +40
===============================================
+ Hits 24657 24852 +195
- Misses 8530 8565 +35
- Partials 2333 2353 +20
|
There was a problem hiding this comment.
@andersleung This looks good. I don't have any comments except for 1 question about a todo that I think might be unecessary now.
| // Only use POST request if supported and there are multiple intervals | ||
| if (this.usePOSTRequest && intervals.size() > 1) { | ||
| // POST request does not guarantee that all returned records are overlapped/contained | ||
| // by requested intervals, so we still need to filter, but don't need to filter duplicates |
| } | ||
|
|
||
| @Override | ||
| public HtsgetPOSTRequest withFormat(final HtsgetFormat format) { |
There was a problem hiding this comment.
It's so frustrating that java doesn't have a way of expressing this without overriding all the methods. It really needs a self type for just this situation.
| @@ -199,6 +211,11 @@ public static void testQueryOverlapped(final HtsgetBAMFileReader htsgetReader) t | |||
|
|
|||
| @Test(dataProvider = "readerProvider") | |||
| public static void testRemovesDuplicates(final HtsgetBAMFileReader htsgetReader) throws IOException { | |||
| // TODO: temporary workaround as reference server does not properly merge regions and remove duplicates yet | |||
There was a problem hiding this comment.
@andersleung Is this fixed now?
There was a problem hiding this comment.
We started the work on this but with the onramp projects (DRS and Passports) and now the file formats work there's probably not going to enough bandwidth to push it through any time soon, so it's not fixed
There was a problem hiding this comment.
Is there an issue # for it we can link to somewhere to monitor the progress? It's one of those things that's confusing in the future if we don't have somewhere to look.
There was a problem hiding this comment.
Created an issue and linked to it in the comment ga4gh/htsget-refserver#27
|
@andersleung Thank you. This is still marked as in progress. Is there any reason we can't merge it? Did I miss some important missing thing when I looked ai it. It seemed pretty sane to me. |
|
Ah sorry just force of habit. It should be ready to merge |
|
Excellent. I'll merge when tests pass which they definitely should because you just changed a comment.. |
Provide classes implementing the new Htsget POST request API, as well as integrate this into the HtsgetBamFileReader class so that it makes use of the API when available.
@lbergelson
Things to think about before submitting: