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

pubsub: support generated subscription names #1799

Merged
merged 2 commits into from
Nov 28, 2016

Conversation

stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented Nov 16, 2016

Fixes #1257

  • removes opts.reuseExisting from topic.subscribe('name', opts)
  • automatically generates a subscription name for the user if they don't provide one

@jgeewax explained the worker vs broadcast model:

A worker takes an item from the message queue, works, acks. The name of the subscription is important to the worker, so the same work isn't performed on the same message multiple times.

The broadcast model is for a listener who just wants to tap into the incoming messages. The name of the subscription doesn't matter in this case.

Before

topic.subscribe('my-subscription', { reuseExisting: true }, function(err, subscription) {
  // subscription was either created or re-used
})

After

topic.subscribe('my-subscription', function(err, subscription) {
  // subscription was either created or re-used
})

To differentiate between the worker and broadcast models, this PR uses the presence of a subscription name in place of opts.reuseExisting. So, if I just want to receive all of the messages:

topic.subscribe(function(err, subscription) {
  // subscription with a random name was created
})

@stephenplusplus stephenplusplus added enhancement api: pubsub Issues related to the Pub/Sub API. labels Nov 16, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2016
* @private
*/
Subscription.generateName_ = function() {
return 'autogenerated-' + uuid.v4();

This comment was marked as spam.

@stephenplusplus
Copy link
Contributor Author

cc: @jgeewax @tmatsuo

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7e2b679 on stephenplusplus:spp--1257 into * on GoogleCloudPlatform:master*.

@callmehiphop
Copy link
Contributor

@stephenplusplus this LGTM, do we want a review from a googler before going with it?

@stephenplusplus
Copy link
Contributor Author

Let's just go with it.

@stephenplusplus stephenplusplus merged commit 2a67398 into googleapis:master Nov 28, 2016
@stephenplusplus stephenplusplus mentioned this pull request Feb 9, 2017
@c0b
Copy link
Contributor

c0b commented Feb 14, 2017

question here: after I changed code from

topic.subscribe('my-subscription', { ackDeadlineSeconds: 90, autoAck: true, interval: 30, reuseExisting: true }).then(...).catch(err => ...)

to just remove the reuseExisting

topic.subscribe('my-subscription', { ackDeadlineSeconds: 90, autoAck: true, interval: 30, }).then(...).catch(err => { console.error(err); })

why it ran into catch error clause?

fail to subscribe:
{ Error: Resource already exists in the project (resource=subscriber-name).
    at /node_modules/grpc/src/node/src/client.js:434:17
  code: 409,
  metadata: 
   Metadata {
     _internal_repr: 
      { 'google.rpc.debuginfo-bin': 
         [ Buffer [
             67,

@stephenplusplus
Copy link
Contributor Author

I tried with the following code and couldn't reproduce using the latest @google-cloud/pubsub:

'use strict'

var pubsub = require('@google-cloud/pubsub')()
var topic = pubsub.topic('stephen-topic')

topic.get({ autoCreate: true })
  .then(() => {
    return topic.subscribe('my-new-subscription', {
      ackDeadlineSeconds: 90,
      autoAck: true,
      interval: 30
    })
  })
  .then(console.log) // fires
  .catch(console.log) // does not

Can you try this code, modifying only the topic name?

@rhagigi
Copy link

rhagigi commented Feb 21, 2017

Question -- if you use an auto-generated subscription name, won't it create a bunch of these every time you start up a new listener? And if the listeners don't cleanly delete the subscription on shutdown, won't those "randomly named" subscriptions still exist but not be drained, queueing up any and all messages on the topic for whatever the max is (7 days I think?) until they're eventually shut down in 30 days? And if it does that, would that affect my GCP quota?

Or is the idea here that you must gracefully shut down and delete the unnamed subscription?

I'm in a situation where I need my node process to just be one of many listeners to a topic and notify any users connected over a websocket if a message comes across that interests them. What's my best way to ephemerally subscribe to a topic without creating a long-lasting subscription?

@fab-1
Copy link

fab-1 commented Apr 25, 2017

Indeed, it does generate new subs for the topic (there is a cap of 10,000 subs per topics I believe)
I would have liked a specific 'broadcast' function at the topic level (instead of publish), but this pull request at least solve the problem - thanks @stephenplusplus

In my case, I just automatically clean up previously generated subscription before subscribing to the topic :

topicObj.getSubscriptions().then(function(subscriptionInstances) {
   if (subscriptionInstances.length) {
      subscriptionInstances[0].forEach((subInstance)=>{
           logging.info('cleaning up subscription ' + subInstance.name);
           subInstance.delete()
      })
   }
});

Not very pretty, but it does the job :)

@rhagigi
Copy link

rhagigi commented Apr 25, 2017

Be careful doing this. At least for me the point of the broadcast model is that you can have multiple listeners at the same time... Be careful that you don't delete another instance's' subscription that is still running.

@fab-1
Copy link

fab-1 commented Apr 25, 2017

Yes, good point @rhagigi . In my case, I run this before deploying, so the new subs get re-created once the instances start. There might be a small risk but I don't know any other options.

@rhagigi
Copy link

rhagigi commented Apr 25, 2017 via email

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 Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants