-
Notifications
You must be signed in to change notification settings - Fork 157
Automatic retry with exponential backoff #58
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
Conversation
oh, i just screw the other http errors, i will be fixing it
cool thanks... let me know when you are ready to review
it's fixed, just removed the error raise middleware, it was causing more problems than helping, refactored to use just retryable_statuses.
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.
just two minor suggestions..
Think it would be better if we put all the error handling into a different module or class, I will fork the repo and make a PR regarding this issue.
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.
👇here why not just using execute_notifications(post_body) instead
lib/fcm.rb
Outdated
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.
since there are four places where we need to call /fcm/notification
- create_notification_key(post)
- add_registration_ids(post)
- remove_registration_ids(post)
- recover_notification_key(get)
maybe it would be better if we refactor this api call into a separate private method, like what execute_notification method does..
def update_group_messaging_setting(body) for_uri(BASE_URI, extra_headers) do |connection| response = connection.post('/fcm/notification', body.to_json) build_response(response) end end
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.
done, except for recover_notification_key, because it's a get request
dgmstuart
commented
May 7, 2019
Hi - great work on the PR. Is there a plan to get this merged soon? We're using the gem and are trying to work out if we can wait for this PR or if we need to create our own fork. Thanks!
Implemented it using the recently added gem Faraday, when it is status 5xx or 200 and any of the results form body is error:Unavaible, it will retry the whole request, with an exponential interval.
passed on simple rspec test with webmock stubs.
I'm not sure if those interval params where good the exponential backoff.
Also updated group notification url to fcm url. #56
ToDo
Implement a Retry faraday middleware that allow editing the body of the request before resend it, so when it is 200 + error:unavaible, we can resend it only for the registration_ids that failed, instead to sending for all.