Skip to content

Conversation

@quartzmo
Copy link
Member

@quartzmo quartzmo commented May 10, 2021

Reference implementations:

@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label May 10, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 10, 2021
@quartzmo quartzmo self-assigned this May 10, 2021
@quartzmo quartzmo force-pushed the pubsub-publisher-flow-control branch 3 times, most recently from 32ed8aa to 0532f46 Compare May 14, 2021 18:59
@quartzmo quartzmo marked this pull request as ready for review May 14, 2021 22:00
@quartzmo quartzmo requested a review from a team as a code owner May 14, 2021 22:00
@quartzmo quartzmo requested a review from kamalaboulhosn May 14, 2021 22:02
@quartzmo
Copy link
Member Author

@plamut PTAL if you have time, thank you.

Copy link

@plamut plamut left a 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 🙂 )

@quartzmo
Copy link
Member Author

@plamut Thank you for all these great suggestions!

@quartzmo
Copy link
Member Author

@plamut PR updated for all your comment, thank you!

Copy link

@plamut plamut left a 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. 🤣

@quartzmo quartzmo force-pushed the pubsub-publisher-flow-control branch from 09026b6 to 5813555 Compare May 20, 2021 15:36
@quartzmo quartzmo force-pushed the pubsub-publisher-flow-control branch from 5813555 to f7ce29a Compare May 20, 2021 18:13
Copy link
Contributor

@dazuma dazuma left a 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!

@plamut
Copy link

plamut commented May 28, 2021

Looks good IMO, but again, I'm not fluent in Ruby. :)

@quartzmo
Copy link
Member Author

@kamalaboulhosn PTAL when you have a chance, thanks!

@quartzmo quartzmo merged commit cfa68ba into googleapis:master Jun 10, 2021
@quartzmo quartzmo deleted the pubsub-publisher-flow-control branch June 10, 2021 15:45
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. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants