1232781 - Python client should honour HTTP 429 rate limit exceeded

Status: RESOLVED WONTFIX

(Tree Management Graveyard :: Treeherder: Client Libraries, defect, P3)

Description

[Ed Morley [:emorley]]

At the moment it just keeps on hammering the API, making the problem in bug 1232776 worse.

Cameron/Will/Mauro: could one of you take a look at this? (Whilst I'll try to work on a few things whilst on PTO, I won't have time for this) :-)

The nodejs client implementation is here:
https://github.com/mozilla/treeherder-node/blob/caaea43fbfab59722b6779b8e5902687aa3c6871/project.js#L73-L82

However it looks like they are using the wrong header, so need to be fixed too:
https://github.com/tomchristie/django-rest-framework/blob/6284bceaaff0e53349131164ce5c16cda8deb715/rest_framework/views.py#L70
https://github.com/tomchristie/django-rest-framework/blob/6284bceaaff0e53349131164ce5c16cda8deb715/docs/topics/3.0-announcement.md#throttle-headers-using-retry-after

-> "The custom X-Throttle-Wait-Second header has now been dropped in favor of the standard Retry-After header."

Comment 1

[William Lachance (:wlach)]

Here's the current retry logic:

https://github.com/mozilla/treeherder/blob/master/treeherder/log_parser/utils.py#L130
https://github.com/mozilla/treeherder/blob/master/treeherder/log_parser/utils.py#L74

It seems like this'll make it retry after a minute, backing off further and further as time goes on. Doesn't seem that bad to me?

Comment 2

[William Lachance (:wlach)]

Ok, so it sounds like the 429 response actually gives a hint on how long the backoff interval should be, which we should theoretically follow. Except that we're (as far as I know) just returning django-rest-framework's default for this, which may or may not be right. I don't think we should change the behaviour here until we're sure of what the implications are. For all we know, honoring this setting might make things worse.