Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Many ways to construct a Subscription without specifying message receipt options #247

Closed
jganetsk opened this issue Sep 7, 2018 · 4 comments
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. triaged for GA type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@jganetsk
Copy link

jganetsk commented Sep 7, 2018

The Subscription constructor takes an important options argument for configuring the receipt of messages: aka batching/flow control

https://cloud.google.com/nodejs/docs/reference/pubsub/0.19.x/Subscription#Subscription

But there are many ways to get a Subscription object without having the ability to specify these options:
https://cloud.google.com/nodejs/docs/reference/pubsub/0.19.x/v1.SubscriberClient#createSubscription (note these are a different set options that are required for the CreateSubscription call, but not for configuring the reciept of messages)
https://cloud.google.com/nodejs/docs/reference/pubsub/0.19.x/v1.SubscriberClient#getSubscription
https://cloud.google.com/nodejs/docs/reference/pubsub/0.19.x/v1.SubscriberClient#listSubscriptions

The Python library does this better... it decouples the Subscription as a noun from the verb of subscribing. Instead of taking these options in via constructor argument, a subscribe method is provided that takes these options: https://googlecloudplatform.github.io/google-cloud-python/latest/pubsub/subscriber/api/client.html#google.cloud.pubsub_v1.subscriber.client.Client.subscribe

However, this can't work given the way things are currently structured in the Javascript library, because the Subscription object is an EventEmitter. Therefore, a Subscription cannot exist without being already configured to receive messages. Alternately, you could make the Subscription object not be an EventEmitter, and (similar to Python) add a subscribe method that takes configuration and returns an EventEmitter.

@theacodes theacodes added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. triaged for GA release blocking Required feature/issue must be fixed prior to next release. labels Sep 7, 2018
@JustinBeckwith JustinBeckwith added the triage me I really want to be triaged. label Sep 8, 2018
@JustinBeckwith JustinBeckwith added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. release blocking Required feature/issue must be fixed prior to next release. triage me I really want to be triaged. labels Sep 12, 2018
@JustinBeckwith JustinBeckwith added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Sep 18, 2018
@callmehiphop
Copy link
Contributor

This seems reasonable to me. @stephenplusplus @jmdobry do you have any feelings here?

@JustinBeckwith JustinBeckwith added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Sep 25, 2018
@stephenplusplus
Copy link
Contributor

I would rather keep things the way they are, in that a Subscription is ready to emit messages whenever the user is. Another way we thought to solve this was adding a setOptions() (better name welcome): #12

@jganetsk
Copy link
Author

jganetsk commented Oct 1, 2018 via email

@JustinBeckwith JustinBeckwith added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Oct 6, 2018
@JustinBeckwith JustinBeckwith added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Oct 16, 2018
@callmehiphop
Copy link
Contributor

I'm going to close this since it appears to be a duplicate of #12

@google-cloud-label-sync google-cloud-label-sync bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. triaged for GA type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

5 participants