search

dashbitco / broadway_cloud_pub_sub

A Broadway producer for Google Cloud Pub/Sub
Apache License 2.0
70 stars 24 forks source link

Add new default Client implementation with Finch #66

Closed elbow-jason closed 2 years ago

elbow-jason commented 2 years ago

Due to many issues with the HTTP adapter stack (a big ol' stack) we decided to implement a Client that relies on Finch.

Feedback is welcome.

closes #51 closes #63

josevalim commented 2 years ago

@elbow-jason this is great!!! I think we can go ahead and remove hackney and the old client too. Let's use the opportunity we haven't reached 1.0 out and get folks to try the finch based client. Thanks!

mcrumm commented 2 years ago

@josevalim Shall we remove the google_api/hackney code in this PR or leave it for a future update?

josevalim commented 2 years ago

This PR IMO!

elbow-jason commented 2 years ago

Crumm and I turned the old GoogleApiClient tests into PullClient tests and removed Telsa. The experience was much fun.

Note: We did not reimplement retries (Tesla had retries). Happy to discuss later.

josevalim commented 2 years ago

Hi @wronfim @michaelst! We are moving away from Hackney+PubSub client in favor of a simpler and more performant stack. However, as it stands right now, there is no retry mechanism, and I believe you needed the retry mechanism in the past. I will merge this PR and it would be very appreciated if you can give it a try and add the retry mechanism back if it is still necessary to you. Thank you!

josevalim commented 2 years ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart:

michaelst commented 2 years ago

My concern that we needed the retry logic for were logger.error messages like this that was added https://github.com/dashbitco/broadway_cloud_pub_sub/pull/66/files#diff-e1aee920e233783f95ecd993e276b2e9774d941a6ef952f8c59eced612258590R88 as google pubsub often returns 502s and we have alerting setup around logger.error messages so it was very noisy for us, maybe those could be changed to logger.warn? They aren't truly errors right because the broadway library will just do another fetch later?

josevalim commented 2 years ago

I believe the logs were a part of it but maybe you also wanted to retry sooner than later? In any case, if the concern is the status, we could make the status code/log level configurable. :) A PR would certainly be appreciated.

michaelst commented 2 years ago

I believe that was part of it, for me at least the more annoying part was the log level, I will see if I can do a PR for that next week