-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add retry on 429 error code #455
Conversation
lambda response: getattr(response, "status_code", None) | ||
in (502, 503, 504), | ||
in (429, 502, 503, 504), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add any delays for the retry? Immediate retry will just make the problem worse (not sure if that's already built in)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t part of the Trino protocol. Before we add this here, it needs to be specified in the protocol, which will require more details. How long should the client wait? Is there a standard header for this?
This behavior needs to be specified in https://trino.io/docs/current/develop/client-protocol.html There is a standard |
d18dae0
to
dfa6840
Compare
trino/client.py
Outdated
@@ -887,7 +895,11 @@ def decorated(*args, **kwargs): | |||
try: | |||
result = func(*args, **kwargs) | |||
if any(guard(result) for guard in conditions): | |||
handle_retry.retry(func, args, kwargs, None, attempt) | |||
if result.status_code == 429 and "Retry-After" in result.headers: | |||
handle_retry_sleep = _RetryAfterSleep(result.headers.get("Retry-After")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a method to convert header value to seconds. The header can be a point-in-time too. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After#syntax in which case this code will throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a method _parse_retry_after_header
for parsing Retry-After
header.
dfa6840
to
872a6f4
Compare
872a6f4
to
0e54f25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the client.
As David said earlier we should document the expectation from other clients for HTTP 429 in the Trino client protocol docs. Please address that in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trino protocol docs were updated. So this is now good to merge.
Description
Add retry on 429 error code
Non-technical explanation
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: