-
Notifications
You must be signed in to change notification settings - Fork 554
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
feat(pubsub): Add Publisher Flow Control #11383
feat(pubsub): Add Publisher Flow Control #11383
Conversation
32ed8aa
to
0532f46
Compare
@plamut PTAL if you have time, thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know Ruby code and tis common idioms, thus mostly just focused on the bigger picture - it looks good IMO! I like the queue of events so that only a single thread is woken up when a message gets released from the flow control.
I might have missed it, but I don't think we release messages when a random network (or other) error happens, please double check (and add a test or two, if needed).
The rest are mostly remarks/suggestions about making the tests more resilient.
(again, apologies for the noise in advance in case I misunderstood the code 🙂 )
google-cloud-pubsub/test/google/cloud/pubsub/async_publisher/message_ordering_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-pubsub/test/google/cloud/pubsub/async_publisher_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-pubsub/test/google/cloud/pubsub/flow_controller_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-pubsub/test/google/cloud/pubsub/flow_controller_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-pubsub/test/google/cloud/pubsub/topic/publish_async_test.rb
Outdated
Show resolved
Hide resolved
@plamut Thank you for all these great suggestions! |
@plamut PR updated for all your comment, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see and understand, I'd say it looks good now, thanks for applying the suggestions.
I trust that the code is idiomatic ruby, and that rubocop thing did not complain. 🤣
09026b6
to
5813555
Compare
5813555
to
f7ce29a
Compare
google-cloud-pubsub/test/google/cloud/pubsub/flow_controller_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-pubsub/test/google/cloud/pubsub/flow_controller_test.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this is looking good!
Looks good IMO, but again, I'm not fluent in Ruby. :) |
@kamalaboulhosn PTAL when you have a chance, thanks! |
Reference implementations: