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

cofigurable retries for HTTP 429 #35

Merged
merged 2 commits into from
Apr 30, 2018
Merged

Conversation

pritamsarkar007
Copy link

@sethgrid please have a look.

@sethgrid
Copy link
Owner

Thanks for the quick fix.

@sethgrid sethgrid merged commit 03e26c9 into sethgrid:master Apr 30, 2018
@jefferai
Copy link

I think this is super awkward API and it'd be better off instead instituting a map of code to retry function, with a default map set by the package. That would be easy and way more flexible.

@pritamsarkar007
Copy link
Author

@jefferai Do you a mean a MAP which can also be configured by the client? If the answer is yes, then does that not make the api vulnerable to the abuse of the feature.

@jefferai
Copy link

Yes, and no.

@pritamsarkar007
Copy link
Author

I thought if you let a user of API to decide which HTTP code to retry, they can abuse and not follow the standard. What are your thoughts on this @jefferai ? @sethgrid any comment?

@jefferai
Copy link

Users are also welcome to not use this library and simply retry on any code anyways. This library provides convenience, not security.

@pritamsarkar007
Copy link
Author

Let's see what @sethgrid has to say about this. How are we going to handle retry for all 5xx error by default using a MAP is what I would like to understand better.
@jefferai Does this still block you to use this library ?

@jefferai
Copy link

You just set up the map with default values on library init.

We already use this library.

@sethgrid
Copy link
Owner

sethgrid commented May 1, 2018

If the use case for altering backoff is concerned with the single 429 status code, then I think a single flag for that single status code, while a bit unwieldy, can just be a wart of the API.

If we want a more elegant solution that allows more fine grained control and a potentially better API, I'd suggest that we have an empty map of client.BackoffOverride[statusCode][backoffStrategy]. The code can just check against the map and and see if the current status code has a new backoff function.

This change would not account for not retrying on 429 however, it would just give you the option to alter the backoff timing. If we further wanted to give power to the backoff function and say a return value of time.Duration(-1) means stop retrying, we can do that. My concern would be that this is a backwards compatibility break, but it is only a small one. I would not expect anyone to have a custom backoff function that returns negative values (as they would just act like a 0 in the current code).

It seems that there are two design questions then: custom backoff per status code available as an override and the separate issue of giving power to the backoffStrategy to return a value that says "don't retry anymore".

@jefferai
Copy link

jefferai commented May 2, 2018

If the map is instantiated at library init time with actual values, instead of "no map entry means default strategy", then it'd be unnecessary to have special return values, as a user could just remove the desired code from the map.

@Richtermeister
Copy link

I'm currently consuming an API which uses status 404 to rate-limit (likely to evade libraries such as this) so I would also be very interested in customizing the retry condition.
Similarly, another API I work with uses headers to indicate how many requests per time period are acceptable, so specifying a callable to determine the retry behavior based on the response would be great. Happy to help implement if this direction is acceptable.

@sethgrid
Copy link
Owner

sethgrid commented May 22, 2018 via email

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.

4 participants