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

auto-created topic and subscribe do not work together #696

Closed
theacodes opened this issue Jun 29, 2015 · 18 comments
Closed

auto-created topic and subscribe do not work together #696

theacodes opened this issue Jun 29, 2015 · 18 comments
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API.

Comments

@theacodes
Copy link

topic.subscribe() on a new topic fails. See this code:

var topic = pubsub.topic('my-topic');  // my-topic hasn't been created yet.
topic.subscribe('my-sub', function(err, _){ console.log(err); }

Will fail with a 404: Resource not found (resource=my-topic),

However, this code works:

var topic = pubsub.topic('my-topic'); 
topic.publish({'dummy': 'message'});
topic.subscribe('my-sub', function(err, _){ console.log(err); }

Seems subscribe doesn't call createTopic if it doesn't exist.

@stephenplusplus
Copy link
Contributor

Should it? Subscribing to a topic that doesn't exist seems like it should be an error, since that subscription would never receive a message event. Like if I try to cat a file that doesn't exist, I wouldn't expect an empty file to be created and then printed to the terminal. If the topic turns out to not exist, I probably meant to subscribe to a different topic, and the error should help point that out.

Creating a topic when a message is published is really just a developer convenience, since the user has always given us the details we needed: "publish message x to topic y", but we were just being stubborn about making them create the topic before we let them publish.

I'm not an expert with Pub/Sub patterns, though, so anyone can set me straight, and it will be easy enough to implement.

@stephenplusplus stephenplusplus added the api: pubsub Issues related to the Pub/Sub API. label Jun 29, 2015
@theacodes
Copy link
Author

@tmatsuo PTAL.

Publishing to a non-existing topic is more dangerous than subscribing to one, in my opinion. Sending a message to a topic with no subscribers just gets dropped. I think we should either auto-create on both or neither for consistency.

This came up on stack overflow with a user following the node.js getting started tutorial. Because our worker runs first and just calls subscribe, the worker will fail. Once the user runs the frontend and triggers a message, then the worker can run successfully. Of course, the message that was used to "prime" the topic is lost forever.

@stephenplusplus
Copy link
Contributor

Publishing to a non-existing topic is more dangerous than subscribing to one, in my opinion. Sending a message to a topic with no subscribers just gets dropped.

If that's true, having autoCreate at all is promoting bad practice. I would prefer we remove the option entirely, since it would always result in a dropped message.

Can you link the SO issue?

@theacodes
Copy link
Author

http://stackoverflow.com/questions/30677889/google-cloud-pub-sub-basic-tutorial-typeerror-cannot-call-method-on-of-null

Tutorial code in question

In terms of having auto create around, I think it's useful per our lengthy discussion when we originally added the feature. However, if there's a way to go back on that decision without making the above code horribly complicated then I'm for it.

@stephenplusplus
Copy link
Contributor

😵 3 months ago feels like 3 years ago. Here's the original discussion and the PR.

I see that @tmatsuo brought up the concern initially:

With the current version of Cloud Pub/Sub, a topic without any subscriptions will throw away your messages until the first subscription will be created and attached to it.

And your response:

I personally think it's a better to document that a topic with no subscriptions doesn't deliver messages and add the autoCreate option than to keep the developer experience we have now. The experience we have now is quite painful and frustrating.

At that time, I believe you were referring to:

pubsub.createTopic('new-topic', function(err, topic) {
  topic.publish({ data: 'message' }, function(err) {});
});

And what we have now:

pubsub.topic('new-topic').publish({ data: 'message' }, function(err) {});

While the first example is more steps, it isn't a common practice & would result in a dropped message, so why try to create a good experience to cater to a bad idea? If the message continued to exist until a subscriber picked it up, then it would make sense to make this a one-liner. Since that's not the case, I think it would be best to revert back to the old behavior and remove autoCreate.

I think this whole thing about wanting to make the code easier is mostly to solve the problem of: "My same script that creates the topic also publishes and subscribes to the topic" - but is this a rational use in the wild, or are topics typically created one place and then used in another?

A comparison here could be uploading a file to a bucket that doesn't exist with Storage. It will fail since I didn't create the bucket first. Assuming I'm running a website that lets users upload files, which I store in gcs, I wouldn't lazily create the bucket; I would have planned ahead and made the bucket, given it proper metadata/caching/privacy settings, etc, then used it in my server script. If I'm running a production app with Pub/Sub, I would think a similar approach would be taken.

In the case of the tutorial code, it would get a little more complex. It might be better to have a step where users create a topic with the Developer's Console UI, then put the name of it in the config file.

@theacodes
Copy link
Author

While the first example is more steps, it isn't a common practice & would result in a dropped message, so why try to create a good experience to cater to a bad idea.

I agree, this an unfortunate situation. There is also the case that this code:

pubsub.createTopic('new-topic', function(err, topic) {
  topic.publish({ data: 'message' }, function(err) {});
});

Isn't safely idempotent, you'd have to specifically handle the case where the topic already exists, pushing more logic into user-space code, something like:

pubsub.createTopic('new-topic', function(err, topic) {
  if(err && err.code == 409) topic = pubsub.topic('new-topic');
  else if (err) { throw err; }
  topic.publish({ data: 'message' }, function(err) {});
});

In python, it's much more succinct (then again, python favors imperative over callbacks):

if not topic.exists():
   topic.create()

but is this a rational use in the wild, or are topics typically created one place and then used in another?

It's absolutely rational in the wild. Subscriptions more so than topics, but still equally valid. For example reliable task scheduling makes each instance create a new subscription. We're also working on a task queueing library for python that allows you to define arbitrary queues that map to topics. @tmatsuo what do you think? I know you originally told me that the "intent" was to have topics and subscriptions pre-created, but I just don't think that's how things are being used at this point.

In the case of the tutorial code, it would get a little more complex. It might be better to have a step where users create a topic with the Developer's Console UI, then put the name of it in the config file.

When the original issue was raised, the UI didn't exist. This could be an option, but it's still onerous to make the users manually do something that should be both possible and simple within the code.

@tmatsuo
Copy link
Contributor

tmatsuo commented Jun 29, 2015

Some applications are OK with fixed topics and subscriptions. Some applications want to create resources (especially subscriptions) dynamically.

I still have a concern that topic's autocreate option is dangerous because it will lead message loss.

How about to have a simple check script which runs on the application startup time, or even on the deploy time, instead of using the "autocreate" feature? I think requirements differ depending on the application and they are different enough for us to be uncomfortable with the single "autocreate" mechanism. The check script may actually create the required resources if it makes sense. Also the check script can decide what to do upon failures (just die, report via e-mail, etc).

@theacodes
Copy link
Author

How about to have a simple check script which runs on the application startup time, or even on the deploy time

I'm opposed to adding an additional step to what is already a very involved tutorial, @stephenplusplus, do you have an recommendations of a simple, reliable, and clear way to do this?

One way I can think of is to add a topic.exists() and topic.create() methods like gcloud-python. Then, I could do this:

function getTopic(cb){
    var topic = pubsub.topic('my-topic');
    topic.exists(function(err, exists){
        if(err) return cb(err);
        if(exists) return cb(topic);
        topic.create(cb);
    })
};

Or createTopic could be idempotent for existing topics and not choke.

If I can find a simple, intuitive way to create or use an existing topic, then the rest of the tutorial code could remain mostly unchanged.

@tmatsuo
Copy link
Contributor

tmatsuo commented Jun 29, 2015

@jonparrott

Is there an easy way for a nodejs app to automatically run some code on startup?
If you add some code for preparing all the resources you need there, that would suffice.

You don't have to change steps in the tutorial. You only need to add some code in the repo.

@stephenplusplus
Copy link
Contributor

@stephenplusplus, do you have an recommendations of a simple, reliable, and clear way to do this?

Not exactly simple, but more clear maybe. I still would recommend using the UI for the tutorial to create a topic. I would prefer to avoid adding functions or modifying behavior just for our Pub/Sub api.

If I'm writing an app, I expect to handle basic things like tracking what I create and re-use. I'm still not clear on why one would publish a message to a topic that doesn't exist. What I've learned here is that's essentially just createTopic().

To go further, I also don't understand how a user gets into a situation where they would publish to a topic that they think exists. The developer should be handling errors, and if they tried to do that, they would get one.

@theacodes
Copy link
Author

I will concede the point and modify the tutorial to instruct the user to create the topic beforehand. I still think that this library should provide a easy, idiomatic way to get or create a topic.

@stephenplusplus
Copy link
Contributor

I want this library to be useful as well, but not at the expense of catering to error-prone/incorrect architectures. Our code will be difficult to maintain as we have to drag along legacy support for use cases that we don't understand or recommend.

Where we're at here:

  1. Remove autoCreate
  2. Decide if we should add a topic.exists() method
  3. Decide if we should make createTopic() create or re-use an existing topic We would then want to do this library-wide (storage.createBucket, index.createDocument, etc)

On point 2, in other places in our library, we usually have object.getMetadata(), which basically serves as an exists(). There seems to be a way to do that for topics: https://cloud.google.com/pubsub/reference/rest/v1/projects.topics/get, but we don't have a topic.getMetadata(). Should we add this method?

@theacodes
Copy link
Author

I definitely think we should add getMetadata(), no need for a exists() in that case.

I'm personally for 3, but I can see the other side of the argument against it.

@dhermes
Copy link
Contributor

dhermes commented Jul 1, 2015

We should probably have a gcloud-meta discussion about this.

Notice getMetadata is just a GET of the resource. Also for other APIs we use fields=name for our exists() request to make a smaller payload than a GET. (But AFAIK pub/sub doesn't support fields.)

@tmatsuo
Copy link
Contributor

tmatsuo commented Jul 1, 2015

Re: fields support

Yup, it's still not working. Just a note, not only Pub/Sub, but also all APIs which are running on the new infrastructure don't support fields field.

@theacodes
Copy link
Author

FYI this commit fixes pubsub in our getting started tutorial. Hopefully nothing we change here will break that.

@jgeewax
Copy link
Contributor

jgeewax commented Jul 1, 2015

If I'm playing the "I'm a lazy developer card", I have a feeling that my code would want to look something like....

  1. Set up gcloud
  2. Get a hold of a Topic object somehow (ie pubsub.topic('my-topic'))
  3. Subscribe to it (if it didn't exist, it seems like it'd be nice to have it magically exist now)
  4. Configure some callback or loop pulling messages off the topic
  5. Publish a message to the topic

Since we're all callback driven, the imperative existence checks and creates kind of suck when you're just trying to get from one step to another (however they're nice when you want to continuously poll for messages and do work whenever a new one comes in).

Given the discussion above, what would the code look like to do this if the "auto-magically create a topic" stuff went away? @stephenplusplus

@stephenplusplus
Copy link
Contributor

Pretty close to https://gist.github.com/stephenplusplus/04594f5026dec99bf896. I'm only saying why would someone want to subscribe to a topic that doesn't exist? This reads to me as "woops! I must have specified the wrong topic name, let me try again", not "I meant for you to create this topic so I can sit here and wait for nothing to happen".

@stephenplusplus stephenplusplus self-assigned this Jul 25, 2015
sofisl pushed a commit that referenced this issue Jan 10, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Jan 24, 2023
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.
Projects
None yet
Development

No branches or pull requests

5 participants