cofigurable retries for HTTP 429

[pritamsarkar007]

@sethgrid please have a look.

[sethgrid]

Thanks for the quick fix.

[jefferai]

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]

@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]

Yes, and no.

[pritamsarkar007]

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]

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

[pritamsarkar007]

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]

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

We already use this library.

[sethgrid]

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]

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]

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]

I think that custom-retry based on headers and/or response codes makes sense, as long as we can implement that in a way that is still performant. I'm not sure when I'll dig up time, but if nobody submits anything, after a while, I can take a look at implementing it and keeping backwards compatibility.

…

On Sun, May 20, 2018 at 11:01 PM Daniel Richter ***@***.***> wrote: 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8afBbugxQfVjBLBYMJZTkEychyU1J0ks5t0lg7gaJpZM4TrKSq> .