Respect the Retry-After header #225
Conversation
There was a problem hiding this comment.
You should be able to use the backoff decorator to implement the 'retry-after' rather than doing it separately: https://github.com/litl/backoff#backoffruntime
|
@snopoke are you happy with this PR? |
| retry_after = response.headers.get("Retry-After", 0.0) | ||
| retry_after = ceil(float(retry_after)) | ||
| logger.warning(f"Sleeping for {retry_after} seconds") | ||
| time.sleep(retry_after) |
There was a problem hiding this comment.
this isn't what I had in mind but I guess it works.
There was a problem hiding this comment.
Another way is to use the wait_gen parameter to give it the time to wait for, but it would need to be context aware. Currently the decorator is not. Do do this, we can wrap this whole get method along with the decorator inside an instance method. That way we can get a wait_gen generator that is able to determine how long to wait for, depending on the Retry-After value.
There was a problem hiding this comment.
Yea, I was thinking of something like that. It might get ugly though. We'd also need to modify the response.raise_for_status() call so that we don't do double backoff.
I'm fine with this for now if you would rather try the other approach later.
There was a problem hiding this comment.
Thoughts on this approach rather? 388a507
|
A unit test would be great |
| @@ -291,6 +291,26 @@ def test_message_log(self): | |||
| [1, 2, 3] | |||
| ) | |||
|
|
|||
| @patch("commcare_export.commcare_hq_client.CommCareHqClient.session") | |||
There was a problem hiding this comment.
Not sure what else we'd want to test?
Retry-Afterheader when it receives a 429 status from HQ.HQ PR to return the
Retry-Afterheader