Handle #925 rate limit error

[stefanoboriero]

• add status code 429 to the list of retriable errors

FIXES #925

[adehad]

Really appreciate you making this, I've added a few comments

(also the GitHub way of referencing an issue would be to use the # symbol and then the number, i.e. #925)

[adehad]

Awesome stuff, just a minor change and otherwise looks good.

---

Regarding:

#1364 (comment)

Same is true for the header "X-RateLimit-Remaining" which is logged with the rest of the headers on line 141, but with the difference that is logged at debug level. If you think that's enough, I'd remove the log of this specific header in the handling of the 429 response code in favour of the more generic header dump

I prefer the current implementation with the specific error message at the warning level. My reasoning would be:

1. It is more like for the log level to be set to info or warning levels than to debug levels.
2. It is a way for the code to document itself, and additionally be more meaningful for the end user than just a dump of headers they need to know the implementation details to understand

But happy to hear your thoughts

[stefanoboriero]

About this comment

I prefer the current implementation with the specific error message at the warning level. My reasoning would be:

It is more like for the log level to be set to info or warning levels than to debug levels.
It is a way for the code to document itself, and additionally be more meaningful for the end user than just a dump of headers they need to know the implementation details to understand

But happy to hear your thoughts

I agree with you, let's keep the more detailed log message at warning level in the status_code handling bit, since as you said it's unlikely users have debug log level enabled when runtime issues like this happen, and we add specific context about retries in the log message.

[adehad]

Awesome, thanks for pushing through this.

[adhamselman]

Hello,
https://github.com/pycontribs/jira/pull/1364/files#diff-0605333df4b31ca7f97b2591e3bcba9c71ea201f4a88e383316e7f6c1c478e06R127
Just wondering, what is the point of only logging the value of 'retry-after' instead of taking it into consideration when implementing the backoff?

[adehad]

@adhamselman
Completely understand where you are coming from, and perhaps there is a further improvement by not bothering to retry if this duration is already greater than the max_retry_delay

[adhamselman]

Not just that. The backoff delay may be calculated to (for example) 20 seconds but the 'retry-after' value may be 30.
I'm not entirely sure but if the retry value is 30 and we retry after 20 seconds, i think the server may penalize you for being too aggressive and increase the wait time.