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

Question: Most Ruby way to create a Pub/Sub topic? #393

Closed
jeffmendoza opened this issue Oct 22, 2015 · 19 comments
Closed

Question: Most Ruby way to create a Pub/Sub topic? #393

jeffmendoza opened this issue Oct 22, 2015 · 19 comments
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. 🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@jeffmendoza
Copy link
Contributor

This library allows a lot of flexibility

# I think I'll go with
topic = pubsub.get_topic $topic_name
if topic.nil?
  topic = pubsub.create_topic $topic_name
end

#----

topic = pubsub.create_topic $topic_name unless pubsub.topic($topic_name).exists?
topic = pubsub.topic $topic_name

#---

if pubsub.topic($topic_name).exists?
  topic = pubsub.topic $topic_name
else
  topic = pubsub.create_topic $topic_name
end

#---

# use autocreate
topic = pubsub.topic $topic_name
topic.publish "message"
# need to create explicitly first to add a subscription though...

#---

cc @thagomizer

@thagomizer
Copy link
Contributor

Pedantic Rubyist Feedback:

What you have is fine but I try to avoid nil? unless necessary since it is an extra method call.

I'd do it this way:

topic ||= pubsub.create_topic $topic_name

You could also do

topic = pubsub.create_topic($topic_name) unless topic

@jeffmendoza
Copy link
Contributor Author

Thanks =)

@blowmage
Copy link
Contributor

FWIW, I agree with @thagomizer that the most idiomatic way to do this in ruby is:

topic = pubsub.get_topic($topic_name) || pubsub.create_topic($topic_name)

However this is not my preferred way. Further in your comments you indicate that you cannot use autocreate because you also need to add a subscription. This is not true. Autocreate will just work in this instance, and both the topic and subscription resources will be created when needed.

topic = pubsub.topic "this-topic-does-not-exist-yet"
sub = topic.subscribe "this-subscription-does-not-exist-yet-either"
sub.name # Now both the topic and subscription exist.

After the second line has run both the topic and subscription will exist. Hope that helps!

@jeffmendoza
Copy link
Contributor Author

topic.subscribe will throw an exception if the subscription already exists though.

It's a bit confusing which operations will create the underlying topic/subscription. Since I can't lazily create the subscription, I need to make sure I have a real topic, then check weather the subscription exists. Now I have:

  topic = pubsub.get_topic $topic_name
  topic ||= pubsub.create_topic $topic_name

  sub = topic.get_subscription $sub_name
  sub ||= topic.subscribe $sub_name

@jgeewax
Copy link

jgeewax commented Oct 23, 2015

This seems like a good place for get_or_create, no?

The users intent is "please give me a topic named this. if it already exists, chances are that was me... just give me that....?"

Needing to write the four lines @jeffmendoza shows makes me sad.

@quartzmo
Copy link
Member

I agree with @jgeewax.

@jgeewax
Copy link

jgeewax commented Oct 23, 2015

To be clear, I could see either of the following being awesome:

topic = pubsub.get_or_create_topic "my-topic"
subscription = topic.subscribe "my-subscription"
# Note that there shouldn't be an error that the subscriptions already exists, right?
# The intent is for me to subscribe. "making that happen" is the same as get-or-create.

or

subscription = pubsub.subscribe "my-topic" "my-subscription"
# Same note as above about no errors...?

@quartzmo
Copy link
Member

An option flag is another, uh, option...

topic = pubsub.topic "this-topic-may-not-exist-yet", create: true
sub = topic.subscribe "this-subscription-may-not-exist-yet-either", create: true
puts sub # Now both the topic and subscription exist.

@blowmage
Copy link
Contributor

It's a bit confusing which operations will create the underlying topic/subscription.

FWIW I agree. We've tried to document that well, and we will take another look at it and see where we can make improvements. Thank you for bringing this up.

Since I can't lazily create the subscription, I need to make sure I have a real topic, then check weather the subscription exists.

You can check if a subscription exists without first creating the topic. I think you can make this a bit easier with the following:

sub = pubsub.get_subscription $sub_name
sub ||= pubsub.topic($topic_name).subscribe $sub_name

I also agree with @jgeewax that a top-level subscribe that could create a topic when needed would be super handy:

sub = pubsub.get_subscription $sub_name
sub ||= pubsub.subscribe $topic_name, $sub_name

@quartzmo
Copy link
Member

I have to agree with @jeffmendoza's opening statement:

This library allows a lot of flexibility

I don't know if we want to re-open this issue or create a new one, but I do think there's something amiss.

I wasn't around when this part of the library was built, and thus come to it with an outsider's perspective. The parallel structure of lazy and non-lazy getters and object state seems very complicated. I am trying to find a justification for it. I can understand the non-lazy path (I want to inspect some attributes of a topic or subscription), but the lazy path raises questions: Will I ever load the full representation? If so, what's the point of not loading it immediately? If not, then why do I load the empty shell in the first place? Is it just so that policy=, publish or subscriptions will have the topic name that I already must have in order that I can pretend to retrieve it?

Is there some use case for actually lazily loading Topic (or Subscription) that I am missing? (There could be, I really am just exploring this...)

Sometimes it helps me to mock up an alternative api in a different domain, so here's an quick attempt to translate this to ActiveRecord and the age-old blog Post and comments. I'm assuming that topic Post.name is unique within scope and is thus equivalent to Topic.name.

With request for Post (equivalent to non-lazy)

post = Post.find_by name:  "my-post-id"
post.comments.create message: "my message content"

Without request for Post (equivalent to lazy)

Comment.create post_name: "my-post-id", message: "my message content"

(One would rarely ever use name instead of id for an association in Rails, but I did in this case for the sake of a closer parallel.)

@jgeewax
Copy link

jgeewax commented Oct 26, 2015

@quartzmo : Wouldn't the lazy Comment.create method fail if it exists already?

The original goal of this auto-create thing was to send a message to a topic 'fire-and-forget' style.

/cc @stephenplusplus from gcloud-node who might be able to chime in, as well as @tmatsuo

@stephenplusplus
Copy link

The original goal of this auto-create thing was to send a message to a topic 'fire-and-forget' style.

I'm not sure if by fire and forget, you mean "creating a topic if it doesn't already exist" here, but worth noting that a message published to a topic without any subscribers is just dropped.

Here's a pretty elaborate discussion on Pub/Sub + autoCreate: googleapis/google-cloud-node#696. @tmatsuo will probably be the most helpful here.

@quartzmo
Copy link
Member

@stephenplusplus Thanks for the link, reading that issue helped me get on board with what you guys are talking about.

@jgeewax I have to apologize, I think my previous comment went off on a tangent from this issue. I found something else that I wanted to discuss that has nothing to do with Topic creation or auto-create.

@jgeewax @blowmage What do you think about a new issue for my complaint that pubsub.topic("my-topic").publish "my-message" is misleading in that it suggests that a topic is retrieved, when the actual function of the topic call is merely to configure the "my-topic" identifier for the subsequent call to publish. The lazy-loading apparatus has no purpose that I can see. (I certainly could be wrong; if so, please let me know how it would be used.)

@jeffmendoza
Copy link
Contributor Author

The no-api-call object create is useful for not adding another round trip, but I think in most cases your code is expecting the topic to exist, and would benefit from the topic.publish (or whatever else you do) to throw an exception if the topic does not exist.

  topic = gcloud.pubsub.topic $topic_name
  topic.publish message
  # We are expecting the topic to have been created earlier.

Since we can't auto-create subscriptions, doing the same with topics makes sense.

  sub = gcloud.pubsub.subscription $sub_name
  # Lazy initialize 
  messages = sub.pull
  # Pull will fail if the sub does not exist

@blowmage
Copy link
Contributor

@jeffmendoza wrote:

The no-api-call object create is useful for not adding another round trip, but I think in most cases your code is expecting the topic to exist, and would benefit from the topic.publish (or whatever else you do) to throw an exception if the topic does not exist.

I 100% agree. This can be accomplished by passing the autocreate flag:

topic = gcloud.pubsub.topic $topic_name, autocreate: false
topic.publish message #=> error will be raised because topic does not exist

@blowmage
Copy link
Contributor

@quartzmo asked:

What do you think about a new issue

I think you have some questions that should be answered, and it probably makes sense to have that discussion on a new issue. :)

@tmatsuo
Copy link
Contributor

tmatsuo commented Oct 26, 2015

There is one thing to note about the current Cloud Pub/Sub service.

If a topic doesn't have any subscriptions, the messages which sent to the topic will effectively get lost.

So:

topic = gcloud.pubsub.topic $topic_name, autocreate: true
topic.publish message
#=> If the topic has just created and there are no subs, this message will go nowhere.

This is raising another annoying issue where you observe some messages will get lost for no obvious reasons, unless you are aware of this fact.

I would recommend that we advertise a clear strategy for ensuring your resources are ready, unless Cloud Pub/Sub changes the behavior.

Just a thought, how about to have some methods for ensuring the existence of resources like must_exist and must_have_subs?

topic = gcloud.pubsub.topic.must_exist $topic_name # It will auto create the topic
# We need to have multiple subscriptions
topic.must_have_subs $sub_array

These are very similar for get_or_create, but they work naturally for both topic and subscriptions (think how we can provide get_or_create for multiple subscriptions). We need to state that no one should use these methods in any loops to avoid additional round trips. Maybe such code should run only once on application startup?

BTW
Re: fire-and-forget
It should not be discussed only for resource creations, but for garbage collections. No?

@blowmage
Copy link
Contributor

@tmatsuo said:

If a topic doesn't have any subscriptions, the messages which sent to the topic will effectively get lost.

I think this is a grave concern. We are going to revisit the Pub/Sub documentation and reverse the order that these concepts are presented. Instead of Topic -> publish -> Subscription -> pull it will be Topic -> Subscription -> publish -> pull. The example code on the methods should also be revisited and always show creating a subscription before a new topic publishes its first message. I think we risk creating confusion if we do not make these changes in our documentation and guidance.

Beyond documentation, this does make me question the usefulness of auto-create for topics. But we really should move that discussion to another topic... hint hint @quartzmo

@quartzmo quartzmo added the api: pubsub Issues related to the Pub/Sub API. label Oct 26, 2015
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Apr 6, 2020
@jeffmendoza
Copy link
Contributor Author

Blast from the past 😃

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. 🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

8 participants