Get final review of Pub/Sub API by Cloud Pub/Sub team from Google #58
Comments
jgeewax
added
type: question
|
@jgeewax Pubsub is merged to master. Who should this be assigned to? |
|
@tmatsuo -- any interest in taking a look at the Ruby Pubsub code? |
|
I'm not proficient with Ruby. |
|
@remi - Any chance you have time to take a look ? If not, we're going to go ahead and cut a release with this ... |
|
https://github.com/GoogleCloudPlatform/gcloud-ruby/blob/master/lib/gcloud/pubsub.rb#L81 |
|
https://github.com/GoogleCloudPlatform/gcloud-ruby/blob/master/lib/gcloud/pubsub.rb#L95 |
|
https://github.com/GoogleCloudPlatform/gcloud-ruby/blob/master/lib/gcloud/pubsub.rb#L133 If the batch call returns multiple msg objects, I feel it's more natural with "msgs" like: msgs = topic.publish do |batch|
batch.publish "new-message-1", foo: :bar
batch.publish "new-message-2", foo: :baz
batch.publish "new-message-3", foo: :bif
end |
|
https://github.com/GoogleCloudPlatform/gcloud-ruby/blob/master/lib/gcloud/pubsub.rb#L176 TYPO: acknoeledged -> acknowledged |
|
https://github.com/GoogleCloudPlatform/gcloud-ruby/blob/master/lib/gcloud/pubsub.rb#L186 How can you create a subscription in project-b, subscribing to a topic in project-a? |
|
https://github.com/GoogleCloudPlatform/gcloud-ruby/blob/master/lib/gcloud/pubsub.rb#L243 Wrong title. Modifying a Message |
|
@tmatsuo said:
Topic#publish returns a single Message object when called without a block, and an array of Message objects when called with a block. See the documentation here. |
|
@tmatsuo said:
The Pub/Sub service takes care of assigning the name, and I assume it makes sure there are no name collisions. |
|
Ok, I just looked at the usage doc in pubsub.rb and put some comments. If the implementation complies to the usage doc, LGTM. Basically I feel it is very well written and provides very natural feeling. Great job. |
|
@tmatsuo said:
We don't allow subscriptions to subscribe to topics in a different project. Is this possible? Is it wanted? |
Yes and yes. It is a very legitimate use case. |
|
@tmatsuo said:
I didn't even think of this, we are setting a default batch size of 100, but I think that was for convenience during development more than anything. I will document the default size in the Subscription#pull documentation. Is 100 a reasonable default? If not, what should it be? |
Okay, right now subscriptions are created on the same project as the topic. We will have to change the API to accommodate this use case. Do you have any suggestions for how the API should allow for this? |
The API doesn't have any default since it's required parameter. |
What about: require "gcloud/pubsub"
pubsub = Gcloud.pubsub # implicitly project-b for example
# it is "projects/project-a/topics/my-topic"
topic = pubsub.topic "my-topic", project: "project-a"
# it is "projects/project-b/subscriptions/my-sub" tied to the above topic
subscription = topic.subscription "my-sub"
puts subscription.name |
Agreed, that is cumbersome. How about this:
|
Yes, it uses the v1 batch API. We moved all of gcloud-ruby to v1 as soon as it was released. |
LGTM |
|
@tmatsuo We will create a new PR for creating a subscription to a topic in a different project. This will require an API change and a separate PR will allow easier discussion of it. All of your other comments, including the ones I have not addressed yet are corrected in #190. Thank you for your attention to detail, it is super appreciated. Let me know if you want a different default value for the number of messages to pull. Right now it is 100 but it can easily be changed. You said:
Thanks! Once we get these two PRs we'll ask for one more sanity check from you before we release. |
|
@blowmage |
|
I've been reading through the new docs and updating a sample of mine to utilize any Pub/Sub features that I haven't already used and I ran into 1 thing:
When I tried to ... messages = subscription.pull immediate: true
messages.each do |msg|
puts msg.message.data.inspect
msg.message.attributes.each do |key, value|
puts "#{key} => #{value}"
end
endI was surprised by the The attributes object returned from the Google API client isn't Enumerable so users have to know to call |
|
Other than attributes, Gcloud Pub/Sub has been pleasant and straight-forward. Two other things surprised me though ... Blocking on Pull
Message objectsThe first time that I pulled messages from a subscription, I tried: messages = subscription.pull # immediate: true
messages.each do |message|
puts "Message ID: #{message.id}"
puts "Message Data: #{message.data}"
endI had to go into the code to realize that an Event object is what is really returned. This results in my code being confusing: messages = subscription.pull
real_messages = messages.map {|not_really_a_message| not_really_a_message.message }... annnnnnnddddd ... I just saw that Event became ReceivedMessage and it delegates |
|
re: Blocking on pull by default, my assumption that the default For waiting for messages, not only would an alias be nice but also a method that blocks indefinitely, yielding retrieved messages to a block when received. eg: subscription.listen do |message|
# this does a Connection#pull.
# when messages are returned, they are yielded and then #pull is called again.
# if the connection fails, #pull is called again.
#
# this provides a way to easily give a block that will continue to listen for
# and process messages until manually breaking out of the loop
endI would need to verify, but node's gcloud lets you say |
|
@tmatsuo We've been working on creating a subscription in one project for a topic in a different project. We actually think the following code works right now, with no code changes: require "gcloud/pubsub"
pubsub = Gcloud.pubsub "project-a", "/path/to/keyfile.json"
# This object holds the connection to the project
puts pubsub.project #=> "project-a"
# We can get a topic in another project (as long as we have permissions)
topic = pubsub.topic "projects/project-b/topics/my-topic"
# When creating a subscription, it uses the original connection and project
subscription = topic.subscription "my-sub"
puts subscription.name #=> "projects/project-a/subscriptions/my-sub"
puts subscription.topic #=> Topic("projects/project-b/topics/my-topic")However, we don't know how to test it. Specifically, we can't figure out how to grant a service account in the first project permissions to the second project. We would really like to verify this works before adding documentation for this, even if it is a manual test and not automated. Can you point us in the right direction for managing these permissions? |
You are correct, we will fix this. It should return a Hash, not a Google API Client object.
This is strange, because the default behavior is indeed to return immediately. In looking into this further I noticed that the API call to Pub/Sub sometimes took a second to return, although the flag to return immediately is set. When setting immediate to
This makes sense. We will add this.
We also agree with this. Although after some design work on our end we suggest a slight change. Your code yielded one message at a time: subscription.listen do |message|
# do the thing
endWe think that it should yield all the messages that are returned from subscription.listen do |messages|
messages.each do |message|
# do the thing
end
endThe reason for this is that we don't have node's nonblocking events, and we don't want to create threads for the users. Our concerns are that we don't want to pull one message at a time because that would be too inefficient. Likewise, we don't want to pull so many messages that the acknowledgement deadline has passed before each message object is yielded. Yielding all pulled messages gives control to the user, and they can use options like threading to process the messages if they want. sub.listen(max: 20) do |msgs|
threads = msgs.map do |msg|
Thread.new do
# do the thing
end
end
threads.each { |t| t.join }
sub.ack msgs
endThoughts? |
My fault! I got this impression when I read "Results can be returned immediately with the :immediate option". Then we show an example that explicitly sets # Results can be returned immediately with the +:immediate+ option:
require "gcloud/pubsub"
pubsub = Gcloud.pubsub
sub = pubsub.subscription "my-topic-sub", immediate: true
msgs = sub.pullIn the Pulling Messages section, can we show an example of using I also didn't realize that
This makes sense so that the developer can ack all of the messages that were received from a particular #pull. Although Node also has an I like ... subscription.listen(max: 20, auto_ack: true) do |message|
# do something
end... because it's simple and I foresee many uses of subscription.listen do |messages|
messages.each do |message|
# do something
subscription.ack message
end
endBut, as you pointed out, this API essentially forces users to send individual ack messages which isn't at all preferable! Although when hmm... |
|
Is |
|
In the Subscription#pull documentation, update the
Should become:
And it might be confusing to say that
Also, what does it mean that
|
|
How about something like this for the Pulling Messages section? Pulling MessagesMessages are pulled from a Subscription. require "gcloud/pubsub"
pubsub = Gcloud.pubsub
subscription = pubsub.subscription "my-topic-sub"
messages = subscription.pullA maximum number of messages returned can also be specified: pubsub = Gcloud.pubsub
subscription = pubsub.subscription "my-topic-sub"
messages = subscription.pull max: 10Messages are returned immediately. If no messages are available, an empty array is returned. Pulling messages can block until a message is available using the +:immediate+ option: pubsub = Gcloud.pubsub
subscription = pubsub.subscription "my-topic-sub"
messages = subscription.pull immediate: falseAn ApiError with status UNAVAILABLE is raised if no messages become available |
Not your fault at all. That is confusing. We have corrected that in PR #192.
I opened issue #81 for auto ack, but no discussion ever came up around it. It is problematic though, I'm personally not sure if we should encourage folks to ack a message before it is handled. Could we move this discussion to that issue?
Ugh, yes, we found that earlier today. So embarrassing...
That is actually very close to the updated docs in #192. |
require "gcloud/pubsub"
pubsub = Gcloud.pubsub "project-a", "/path/to/keyfile.json"
# This object holds the connection to the project
puts pubsub.project #=> "project-a"
# We can get a topic in another project (as long as we have permissions)
topic = pubsub.topic "projects/project-b/topics/my-topic"
# When creating a subscription, it uses the original connection and project
subscription = topic.subscription "my-sub"
puts subscription.name #=> "projects/project-a/subscriptions/my-sub"
puts subscription.topic #=> Topic("projects/project-b/topics/my-topic")
For now, the naive way to do this is to add the service account (or accout) to the other project with an Editor or greater permission. Soon (it is actually in production, but no documentation), you can set finer grained ACL on topic/subscriptions. First, look at the gmail push notification example at (Grant publish rights on your topic): In this case, you're authorizing the service account owned by gmail team, to publish messages to that topic. The role "roles/pubsub.publisher" allows it. As you see in this example, now you can attach various fine grained roles on your topics/subscriptions by calling setIamPolicy method. If you want to create a subscription in project-a, subscribing to a topic in project-b, you can use the role "roles/pubsub.subscriber". In this case, the example will become: For now, there is not handy tool for managing the ACLs, but you can use the API Explorer at: topic = pubsub.topic "projects/project-b/topics/my-topic"Does the above code issue an API call to Cloud Pub/Sub? |
Thanks! We will work on adding cross-project permissions first thing Monday morning. Hopefully we can demonstrate this working without any code changes.
This does not make an API call to Cloud Pub/Sub. If you were calling |
Great :) topic = pubsub.topic "projects/project-b/topics/my-topic"I think it is cleaner to have the topic = pubsub.topic "my-topic", project: "project-b"Sorry if I'm writing wrong code, but what do you think? |
I see that |
|
@blowmage |
|
@tmatsuo Okay, we will add support for getting and setting the policy. We also see |
|
@blowmage I will update here once we have done with the documentation. |
|
@tmatsuo We are adding methods to get and set the policy on topic and subscription, using the service definition as our guide. Right now they are returning and accepting a hash of values which match what the Google API client is using. We can either document the hash structure in the gcloud-ruby documentation, or we can mark these methods hidden so they don't show up in the docs until you have the main documentation settled. Which would you like us to do? (The methods have been helpful in setting policies for testing, so we would like to keep them if possible.) |
|
Either is fine :) |
|
Thanks for your valuable feedback! Please open new github issues for future requests. ❤️ |
Placeholder issue until we are ready for a review
The text was updated successfully, but these errors were encountered: