-
Notifications
You must be signed in to change notification settings - Fork 11
Shard readGroupSets using the associated referenceSet. #64
Conversation
public static Iterable<Reference> getReferences(String referenceSetId, GenomicsFactory.OfflineAuth auth) | ||
throws IOException, GeneralSecurityException { | ||
Genomics genomics = auth.getGenomics(auth.getDefaultFactory()); | ||
Iterable<Reference> references = Paginator.References.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local variable seems unnecessary here.
Thanks for the feedback @calbach. PTAL I switched to Coverage Buckets for read group set sharding. Its much better to use something available for all read group sets. I left the getReferenceId and getReference methods in the utility class since they're generally useful. |
public static List<CoverageBucket> getCoverageBuckets(String readGroupSetId, GenomicsFactory.OfflineAuth auth) | ||
throws IOException, GeneralSecurityException { | ||
Genomics genomics = auth.getGenomics(auth.getDefaultFactory()); | ||
// Not using a Paginator here because requests of this form return one result per |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree that this is a safe assumption. If there is already a paginator I would strongly prefer it were used. Otherwise, you should at least throw an exception if nextPageToken is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no Paginator for that method. I added the exception.
LGTM |
genomics.readgroupsets().coveragebuckets().list(readGroupSetId).execute(); | ||
// Requests of this form return one result per reference name, so therefore many fewer than | ||
// the default page size, but verify that the assumption holds true. | ||
if(null != response.getNextPageToken()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space after 'if'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strings.isNullOrEmpty() would probably be safer
LGTM
|
Shard readGroupSets using the associated referenceSet.
Take a look at deflaux/dataflow-java@33f0cff so see how this change could be used downstream. (but I plan to refactor that refactor; it needs a bit more work)