Skip to content
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

Retry up to N times in case of 429 response #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Retry up to N times in case of 429 response #34

wants to merge 2 commits into from

Conversation

flimzy
Copy link
Contributor

@flimzy flimzy commented Dec 20, 2016

This fixes #31 .

Additional discussion may be warranted before a merge, though, in case the default behavior should possibly be changed.

As of now, I simply expose a hook.Retries field, which can be set to non-zero to enable retries. Otherwise, behavior remains the same as before.

This in preparation for running in a loop to handle retries.
@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+1.004%) to 83.702% when pulling 9cd97e9 on flimzy:retry into 3d7f059 on evalphobia:master.

@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+1.004%) to 83.702% when pulling b37bcdd on flimzy:retry into 3d7f059 on evalphobia:master.

@mcpherrinm
Copy link

It seems to me that you'd want to do some kind of exponential backoff here. If Sentry thinks it's overloaded, hammering it repeatedly seems like the wrong thing to do.

@evalphobia
Copy link
Owner

@flimzy Thank you for contributing. And sorry for my lateness.
429 error is the kind of error for rate limit, right?

As @mcpherrinm mentioned, I think it needs some waiting time to guarantee the effectiveness.
For example, HTTP library h2non/gentleman has plugin for retry and it implements waiting time to retry.
It use millisec scale, but Sentry's rate limit seems calcutlates the events on each minute, so the scale should be more than seconds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support retries in case of 429 response
4 participants