Skip to content
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

zulip.Client.call_endpoint: Add retry_on_rate_limit_error. #705

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ro-i
Copy link
Contributor

@ro-i ro-i commented Jun 28, 2021

Hi :)

I'd like to suggest a new option for the call_endpoint method of zulip.Client. It's something I added to my bot. @timabbott suggested to also consider this for the API, see https://chat.zulip.org/#narrow/stream/378-api-design/topic/Rate.20limits/near/1217048 for the discussion.

Short description of the patch:
If the call_endpoint method is called with the retry_on_rate_limit_error parameter set to true, wait and retry automatically on rate limit errors.

timeout=timeout,
)
if not retry_on_rate_limit_error or result["result"] == "success":
break
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do this as two conditionals, first the success one, and then the other one. We like to do this so that code coverage tools can tell you whether we have tests for each independent condition.

elif "code" in result and result["code"] == "RATE_LIMIT_HIT":
secs = result["retry-after"]
elif "X-RateLimit-Reset" in result:
secs = float(result["X-RateLimit-Reset"])
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think the retry-after property is there since the beginning of time for these errors; just not code. So I think you could do

if 'retry-after' not result:
    break
 
wait_before_retry_secs = result["retry-after"]
logger.warning(...)
time.sleep(...)

@ro-i ro-i force-pushed the retry-on-rate-limit-error branch from b870097 to 0d26501 Compare July 2, 2021 20:24
@ro-i
Copy link
Contributor Author

ro-i commented Jul 2, 2021

@timabbott Thanks for the feedback! 👍
I updated the code (the layout of the if-statement with the closing bracket on its own line has been done by ./tools/lint btw. ;) ).
And you would also think that retry-after has always been a float, not a string? I'm not sure how that has been handled by older Zulip versions. And I'm not sure what happened to the capitalized Retry-After I found here...

If the call_endpoint method is called with the
"retry_on_rate_limit_error" parameter set to true, wait and retry
automatically on rate limit errors.
See https://chat.zulip.org/#narrow/stream/378-api-design/topic/
Rate.20limits/near/1217048 for the discussion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants