Skip to content
This repository has been archived by the owner on Oct 29, 2023. It is now read-only.

Adding classes to assist with streaming from gRPC #99

Merged
merged 2 commits into from
Jun 22, 2015

Conversation

Careyjmac
Copy link
Contributor

Please do not merge just yet, waiting on an update to the utils-java dependency that fixes a bug in gRPC that should make this work successfully on larger datasets.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@pgrosu
Copy link

pgrosu commented Jun 17, 2015

This is very exciting! Will there be gRPC for Annotations too?

@dionloy
Copy link
Contributor

dionloy commented Jun 17, 2015

Hi Paul,

Not quite yet, we're waiting to see how the GA4GH discussions regarding an
official Annotations API coalesces.

On Wed, Jun 17, 2015 at 11:25 AM, Paul Grosu [email protected]
wrote:

This is very exciting! Will there be gRPC for Annotations too?


Reply to this email directly or view it on GitHub
#99 (comment)
.

@pgrosu
Copy link

pgrosu commented Jun 17, 2015

Hi Dion,

I definitely would not wait too long - actually I would not wait at all.

It would definitely be better to just implement one or more proof-of-concept(s) to show their pro/cons, and then tweak them afterwards to GA4GH's preference. Things usually move faster that way, and then people have ideas to play with which are actually applied.

I'm not sure everyone there has the resources and wealth of ideas you would be exposed to at Google. This would be a great leadership opportunity to expand people's minds on how to think differently and/or bigger.

Paul

@Careyjmac
Copy link
Contributor Author

Ok, this should be good to merge now that I was able to update the utils-java version in the pom file.

@dionloy
Copy link
Contributor

dionloy commented Jun 19, 2015

LGTM! Thanks!

On Fri, Jun 19, 2015 at 2:01 PM, Careyjmac [email protected] wrote:

Ok, this should be good to merge now that I was able to update the
utils-java version in the pom file.


Reply to this email directly or view it on GitHub
#99 (comment)
.

Careyjmac pushed a commit that referenced this pull request Jun 22, 2015
Adding classes to assist with streaming from gRPC
@Careyjmac Careyjmac merged commit 16c730d into googlegenomics:master Jun 22, 2015
* This step exists to emit the individual reads in a parallel step to the StreamReads step in
* order to increase throughput.
*/
private static class ConvergeReadsList extends DoFn<List<Read>, Read> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit (and please feel free to ignore): the function name ConvergeReadsList did not convey the function's purpose to me at first glance. I looked up the dictionary definition for "converge" and perhaps it is appropriate in this case because the scenario is many lists coming together as a collection of individual reads?

Alternative names could be ConvertReadsListToReads or EmitIndividualReads.

@deflaux
Copy link
Contributor

deflaux commented Jun 22, 2015

@Careyjmac these helper classes are great! Other than my minor comments, this LGTM.

jiridanek pushed a commit to jiridanek/dataflow-java that referenced this pull request Jan 18, 2016
Adding classes to assist with streaming from gRPC
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants