What's the point of Subscription.get()?

[jganetsk]

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

Subscription.get() returns a Subscription object. Why would I need to call get() on a Subscription object to get a Subscription object when I already have a Subscription object?

[stephenplusplus]

It populates the metadata property with the remote description of the object. It's also useful for the { autoCreate: Boolean } option. The method, along with exists(), getMetadata(), and setMetadata() is consistent across all of the nodejs-* libs.

History and context: googleapis/google-cloud-node#862 (comment)

@theacodes you marked as release blocking, but I don't think we need any changes here. I'll let you make that final call before I close, though!

[jganetsk]

Seems confusing to me. How do you intend people to use this? With code like...

var subscription = ...; subscription = (await subscription.get())[0];

How would they know whether they needed to call get()?

It seems more appropriate to have a method called getSubscription, something like

var subscription = PubSub.getSubscription();

[theacodes]

you marked as release blocking, but I don't think we need any changes here. I'll let you make that final call before I close, though!

This is feedback from the reviewing pub/sub engineer, we need to "resolve" it before we can do 1.0. :)

[stephenplusplus]

I think the most common way to use a Subscription object would be to get a reference to one:

const subscription = topic.subscription('name')
...use the subscription...

I don't think they ever need to call get()-- it's a convenience method. If their application is uncertain if the subscription exists, get({ autoCreate: true }) could be useful, so their application can just get a subscription and move on. Ignoring the autoCreate feature, using subscription.get() is probably not that commonly-needed. I think its utility is mostly in consistency with the other @google-cloud/nodejs-* libraries.

Regarding:

var subscription = PubSub.getSubscription();

This requires slight modification, as getSubscription() would be an async call:

var PubSub = require('@google-cloud/pubsub')
var pubsub = new PubSub()

pubsub.getSubscription('name', function(err, subscription) {
  // subscription is a Subscription object
  // subscription.metadata was set with the resource description from the API
})

That's not a bad idea, however, I'm not sure it would be better than the sync way:

var PubSub = require('@google-cloud/pubsub')
var pubsub = new PubSub()

var subscription = pubsub.subscription('name')

The second way has the benefit of not making any API calls. It leaves it up to the user to decide if they care about getting the metadata (via subscription.getMetadata()).

[jganetsk]

I should note that auto-creating a subscription is not a common Pub/Sub use case. Subscriptions hold data. If the user is uncertain about whether their subscription exists, creating a subscription doesn't get their data back for them.

A good analogy is that of using a database. If you want to query a database table, you should be sure the database table already exists. If you auto-create a table, it will be empty, and it will not have the data you were looking for. So database libraries don't auto-create tables whenever you want to query from one.

It's the same with Pub/Sub subscriptions. Creating a new one will result in a subscription with an empty backlog. It will not contain any messages published prior to the creation of a subscription.

[stephenplusplus]

It will not contain any messages published prior to the creation of a subscription.

But, would it receive future messages?

This is familiar to the discussion about if the client library or API should auto-generate names for subscriptions, so that users can start listening for new messages as quickly as possible.

[jganetsk]

Yes, I remember this discussion. I think we decided that auto-generating names was wrong for the reasons I described above.

It would receive future messages. Just like a newly created database table would potentially get future row inserts. But a database library where you can't access any data that was inserted prior to starting up the client isn't a very good way to use a database. The same with Pub/Sub.

We don't want to encourage anti-patterns, though auto-creating subscriptions is better than auto-generating names. But also, afaik, none of the other client libraries support auto-create functionality. Why is NodeJS special?

[stephenplusplus]

The other libs have get({autoCreate}). Here are a few examples:

• Bigtable Family: https://github.com/googleapis/nodejs-bigtable/blob/09e0b5c09b97fb3672decf29e3c7969545c88247/src/family.js#L264
• BigQuery Dataset: https://github.com/googleapis/nodejs-bigquery/blob/ba39ed2604d92f770d55eeb36487803e8f698aab/src/dataset.ts#L106
• Compute Engine Instance: https://github.com/googleapis/nodejs-compute/blob/636a3f697682c935fc52aa60218b27b6e952c66c/src/vm.js#L175

The common set of methods (exists, get, getMetadata, and setMetadata) are all available to the ServiceObject class, which is what all of the above (and many others) are.

I think the use case for "just listening for new messages" is more common than "just querying new data".

[jganetsk]

Of course. "Listening for new messages since the last message my subscriber processed" is very much a pattern. "Listening for new messages since the subscriber restarted" is very much an anti-pattern and our users will be unhappy.

In other words, let's say I'm running a subscriber on my laptop. I reboot my laptop, and it takes 5 minutes to reboot. Those 5 minutes of messages would be lost if I created a new subscription after reboot. And I want those 5 minutes of messages. And if the user is uncertain as to whether they want them, or thinks they don't want them, we still should be on the safe side and deliver those messages.

We don't want users having arbitrary gaps in their data. If a subscription is NOT_FOUND, we don't want to just silently re-create it. We want to alert the user that their subscription was deleted, potentially along with messages.

Note that auto-creating a GCE instance is a bit different, since that's not data storage. And auto-creating things if you want to write to them make sense. The libraries above might be more of a read-write context. But I don't think you can show me an example of a library that reads data where auto-create makes sense.

And also, why is NodeJS special?

[stephenplusplus]

In other words, let's say I'm running a subscriber on my laptop. I reboot my laptop, and it takes 5 minutes to reboot. Those 5 minutes of messages would be lost if I created a new subscription after reboot. And I want those 5 minutes of messages. And if the user is uncertain as to whether they want them, or thinks they don't want them, we still should be on the safe side and deliver those messages.

This is a great description that helps me understand your view a lot. So, thanks! I think I would agree that if the user knows the name of a subscription, they should know if it exists. And if they're just trying to create a subscription, that should be done through our methods that handle that explicitly.

If a subscription is NOT_FOUND, we don't want to just silently re-create it.

That's something we don't do silently, in that the user has to set autoCreate: true manually.

And also, why is NodeJS special?

Any more context?

[jganetsk]

I don't think we have autoCreate functionality in the Pub/Sub client libraries of other languages.

Moving back to the original discussion, I think subscription.get is confusing. It is just odd that you call get on a Subscription object to get a Subscription object. You already have a Subscription object, why do you need another Subscription object? Isn't there a chicken-and-egg problem here? To call the get method, you need to already have a Subscription object. But then... how did you get that one? Did you call subscription.get to get that one? Is it turtles all the way down? Therefore, there must be some way to get a Subscription object outside of subscription.get. Something like PubSub.getSubscription(). So if that method exists, why do we need subscription.get?

I see the need for exists , getMetadata, and setMetadata. But just not for get.

[stephenplusplus]

I don't think we have autoCreate functionality in the Pub/Sub client libraries of other languages.

It all started back in 2015 after a request by @jgeewax, who at the time, was the most involved in our development. I'm not sure how far he cast that suggestion or why it only hit Node, if it did.

The utility in get() is that it gives you autoCreate. Without it, it's not very useful, other than it will ping the API to get and populate the metadata property on the instance. We have always had sync methods to access ServiceObjects (e.g. Subscription or File from GCS). That's so that we don't make any API calls, and the user doesn't have to deal with any callbacks just to get started with the API. The first async/API call is for the operation they actually care about, and not just plumbing.

As we go on, it's bringing back memories on the history of these decisions. I believe one reason for having get() was so that our documentation could show a user who lands on the page for the concept they're interested in, how to use it. In other words, if a user wants to interact with a GCS Bucket, they'll probably navigate to the "Bucket" page of our docs, as opposed to the parent object, "Storage". Since our docs are a method-by-method API reference, as opposed to a guided walkthrough, it was considered helpful to have accessor methods, so that they could be found on that page with inline examples.

I'm hesitant to create get{ServiceObject} methods, as it would showcase a method that I don't think will be that useful, and open questions to users about which they should use, api.serviceObject() vs api.getServiceObject(). I recognize api.serviceObject.get() stands to confuse them as well, however:

1. It has a unique feature, "autoCreate"
2. It's not on the same API documentation page

To resolve this issue, should we get rid of get() for Topic and Subscription? Based on your earlier points, I think it's harmful for Subscription, and less so for Topic, but defer to your judgement.

[jganetsk]

I will repeat what I said earlier, which you did not address with your comment:

Isn't there a chicken-and-egg problem here? To call the get method, you need to already have a Subscription object. But then... how did you get that one? Did you call subscription.get to get that one? Is it turtles all the way down? Therefore, there must be some way to get a Subscription object outside of subscription.get. Something like PubSub.getSubscription(). So if that method exists, why do we need subscription.get?

There are other ways to make subscription objects, like the constructor: https://cloud.google.com/nodejs/docs/reference/pubsub/0.19.x/Subscription#constructor_1

So you can expose autoCreate there. Make it a constructor option. There's no fundamental reason why subscription.get() is necessary. autoCreate can be an option on any function that returns Subscription objects.

And actually, most ways to get a subscription don't expose the options that the constructor already does (like batching and flow control), and that seems like a bug. I filed another issue about that: #247

[stephenplusplus]

Therefore, there must be some way to get a Subscription object outside of subscription.get.

That way is var subscription = pubsub.subscription(). That's the way. pubsub.subscription().get(callback) is a way to use autoCreate and populate the metadata.

[stephenplusplus]

... accidental click on "Close and comment".

Continuing...

There are other ways to make subscription objects, like the constructor

That's actually not supported. Subscriptions must be created through pubsub.subscription(). I suppose a user could figure out how to do that, but it's not the intended way to use it. That's a byproduct of the JSDocs-style documentation, where each class gets its own page, and the constructor is a publicly-documented place to put an introductory description.

autoCreate can be an option on any function that returns Subscription objects.

That's true, we could have autoCreate: true be an option to pubsub.subscription(). We could wait for the user to make their first API request, and then do a quick "wait, do we need to create this?" check for existence, then do it if so. One thing that could be weird there is error handling. If creating fails for some reason, the call to subscription.getMetadata() for example, could receive a seemingly-unrelated error. It's nice to have that "autocreate" behavior contained in one call.

To restate where I'm coming from, I don't think many people are using subscription.get(). I think they know how to use a Subscription directly, e.g. pubsub.subscription(). Speaking generally about the hierarchy concerns, I don't think it's harmful to have the method.

[theacodes]

Okay, I spent a few minutes reading over this thread and the code itself.

It seems to me (and please correct me if I'm mistaken) that this is a remnant of our previous resource-class oriented design, and the new client takes on more of a rpc-method oriented design.

For example, resource-class stuff looks like this:

let topic = pubsub.topic('my-name');
topic.create();

vs rpc-method:

pubsub.createTopic('my-name');

It seems to me that everything except subscription in this client has moved to rpc-method. I would strongly prefer that we address that. An interface such as this would be pretty acceptable to me:

subscriber.createSubscription(...); // Create a subscription.
let subscription = subscriber.getSubscription(...); // Get subscription metadata.

// subscribe to messages.
subscriber.subscribe(subscription.name, callback);

[theacodes]

Fwiw, this is the approach the Python client takes. It does not conflate the subscription "data" with the "eventemitter" that constitutes opening a streaming pull on the subscription. The latter is a distinct action (subscribe).

[stephenplusplus]

I'll be honest, those terms are new to me, but we've always been structured like this:

PubSub
- > Topic
- - > Subscription

Subscriptions can be accessed without a Topic, however, so we have:

PubSub
- > Subscription

To create a subscription:

var pubsub = new PubSub()
pubsub.createSubscription(function(err, subscription) {})

To reference a subscription:

// None of the following makes API calls
var pubsub = new PubSub()
var subscription = pubsub.subscription('name')

And to use the Subscription:

var pubsub = new PubSub()
var subscription = pubsub.subscription('name')
// First API call:
subscription.getMetadata(function(err, metadata) {});

A long time ago, in a not-strictly PubSub-related conversation, it was decided to add some sort of "get or create" convenience methods across all of the APIs that we have Node.js libraries for. That ended up being create() and get() with an autoCreate option. So, we have:

var pubsub = new PubSub()
var subscription = pubsub.subscription('name')

// Create:
subscription.create(function(err, subscription) {})

// Get or create:
subscription.get({ autoCreate: true }, function(err, subscription) {})

That was a secondary design decision, influenced by @jgeewax. We have always prioritized the "Parent#createChild()" hierarchy (in fact, the child.create() method simply aliases the parent.createChild() method).

To get back to your example:

- subscriber.createSubscription(...); // Create a subscription.
+ pubsub.createSubscription('name', function(err, subscription) {}); // Create a subscription.

- let subscription = subscriber.getSubscription(...); // Get subscription metadata.
+ let subscription = pubsub.subscription('name'); // Get subscription object.
+ subscription.getMetadata(function(err, metadata) {}); // Get subscription metadata.

// subscribe to messages.
- subscriber.subscribe(subscription.name, callback);
+ subscription.on('message', function(err, message) {});

[theacodes]

I'm not understanding why we need "subscription" and "topic" classes at this point. They should be POD types.

[stephenplusplus]

I believe to contain their respective functionality.

A few from Subscription: subscription.modifyPushConfig(), subscription.seek(), subscription.setMetadata().

A few from Topic: topic.getSubscriptions(), topic.getMetadata(), topic.createSubscription().

[theacodes]

Be we already have subscriber.createSubscription, modifySubscription, getSubscription etc.

[stephenplusplus]

Do you mean in the generated library? In our handwritten library, we do have a class called Subscriber, but its functionality is within our Subscription class.

[theacodes]

Hrm. Okay. Let me think on this a bit more.

…

On Mon, Sep 10, 2018, 1:23 PM Stephen ***@***.***> wrote: Do you mean in the generated library? In our handwritten library, we do have a class called Subscriber, but its functionality is within our Subscription class. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#248 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPUc0T_-W1BgRFjF5cteE7nWS4gPhNIks5uZsoxgaJpZM4WfiaA> .

[jganetsk]

You suggest:

var pubsub = new PubSub()
var subscription = pubsub.subscription('name')

// Create:
subscription.create(function(err, subscription) {})

// Get or create:
subscription.get({ autoCreate: true }, function(err, subscription) {})

Why not just have a method called createIfNotExists? Isn't that clearer than get?

[stephenplusplus]

I don't mind the "get" / "get no matter what" (autocreate) naming philosophy. It's been around for 3 years, and I can't speak for the users who have this written into their applications and are getting along just fine (...or the ones who don't use it and/or don't like it). Since this is in all of the APIs we support, if you feel strongly this needs to be re-considered, we should probably move the convo to our "nodejs-common", and get more eyes on it.

[jganetsk]

I don't have a problem with get() in general. It just seems obvious to me that an API shouldn't have a getter that returns an object of the same type as this.

Can you point me at APIs that do this, in the wild? Like does the string class in any programming language you know have a method called get() that returns a value of string type? Usually get() methods exist on types that are boxes of some sort... and they return what's inside the box. I've never seen a get() that returns the box itself. Like get() on unique_ptr<T> returns a T*. It would make no sense for it to return a unique_ptr<T>. And I don't think Javascript is an exception here.

But maybe what I find obvious is not necessarily obvious.

[stephenplusplus]

I opened up googleapis/nodejs-common#234 to try to sum up where we're at, and seek more feedback. Feel free to add anything.

[jganetsk]

Ok, this is the lowest priority of the 4 issues I reported. This is a style issue, and I ultimately defer to you and Thea on this.

[stephenplusplus]

Do you think we should remove the auto-create functionality of a Subscription (Topic, as well?)

[jganetsk]

I think it's fine to leave auto-create in there as long as it's non-default. We should make sure the documentation is clear that creating a new subscription means not receiving any messages published prior to creation. And if user's intention is to resume from where they left off last, they should beware using auto-create.

[stephenplusplus]

This has evolved into a core issue, googleapis/nodejs-common#234, so I think it's safe to close here until we figure out which direction we're moving in.