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

Feature: auto retry when rate limit exceeded or server error encountered #86

Merged
merged 12 commits into from
Jan 22, 2024

Conversation

schelv
Copy link
Contributor

@schelv schelv commented Jan 11, 2024

This PR introduces a mechanism that limits the number of concurrent requests.
Additionally, if a RateLimitExceeded response is encountered, new requests are not started for a while.
I'm not sure if halting the new requests is really needed, but it seems the right thing to do.

The print statements are there currently for seeing that the mechanism works.
Should they be replace with a logger? or removed entirely?

Related to #66

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (4406cc0) 35.23% compared to head (a76f8ef) 35.25%.
Report is 1 commits behind head on master.

Files Patch % Lines
githubkit/core.py 42.85% 16 Missing ⚠️
githubkit/retry.py 63.88% 13 Missing ⚠️
githubkit/config.py 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
+ Coverage   35.23%   35.25%   +0.01%     
==========================================
  Files        2414     2416       +2     
  Lines      124893   124971      +78     
==========================================
+ Hits        44007    44053      +46     
- Misses      80886    80918      +32     
Flag Coverage Δ
unittests 35.25% <61.44%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yanyongyu
Copy link
Owner

It seems concurrent restriction should consider both sync/async usage and multi-instance usage. The concurrent restriction mechanism may need external service like redis. Currently, i would like to add a simple retry logic when rate limit exceeded or server error encountered.

@schelv
Copy link
Contributor Author

schelv commented Jan 12, 2024

It seems concurrent restriction should consider both sync/async usage and multi-instance usage. The concurrent restriction mechanism may need external service like redis. Currently, i would like to add a simple retry logic when rate limit exceeded or server error encountered.

Considering both sync and async usage should be possible. I will try to add that.
Taking multi-instance usage into consideration at the mechanism level seems a lot more complex.
A simplistic way to "consider multi-instance usage" without hitting too many rate limits, is by limiting the number of concurrent connections for each of the instances.
Alternatively the number of concurrent connections could be lowered when approaching the primary rate limit. This would leave some requests for other instances. But I'm not sure if this is something that should be handled in this mechanism.

The benefit also depends a lot on the what each of the instances is doing with the api.
Do you have a scenario/use-case in mind with multiple instances?

@yanyongyu
Copy link
Owner

yanyongyu commented Jan 13, 2024

multiple instances are commonly used in multi-process mode or cluster mode. octokit also uses redis to schedule the requests in cluster mode. Maybe an abstract storage layer should be implemented to cache infos and restrict the concurrency. I will try to implement an in-memory storage and a redis one.

@schelv
Copy link
Contributor Author

schelv commented Jan 13, 2024

multiple instances are commonly used in multi-process mode or cluster mode. octokit also uses redis to schedule the requests in cluster mode. Maybe an abstract storage layer should be implemented to cache infos and restrict the concurrency. I will try to implement an in-memory storage and a redis one.

The advised "best practice" is to not do anything concurrently at all.
Is that something that you want to have?
It sounds very very slow.

@yanyongyu
Copy link
Owner

octokit uses the bottleneck library to schedule the request job. you can see the rate limit logic in the library.

@yanyongyu
Copy link
Owner

Due to the complexity of this feature, I'm going to split it into two PRs. In this pr, rate limit auto retry will be implemented and the concurrency limit will be implemented in the next version

@yanyongyu yanyongyu added the enhancement New feature or request label Jan 15, 2024
@yanyongyu yanyongyu changed the title add rate limiting mechanism Feature: auto retry when rate limit exceeded or server error encountered Jan 15, 2024
githubkit/core.py Outdated Show resolved Hide resolved
@yanyongyu yanyongyu merged commit 03d0e56 into yanyongyu:master Jan 22, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants