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

Fixing rate limiter #14

Merged
merged 2 commits into from
Sep 25, 2024
Merged

Fixing rate limiter #14

merged 2 commits into from
Sep 25, 2024

Conversation

arnasbr
Copy link
Contributor

@arnasbr arnasbr commented Sep 25, 2024

My fix here #13 for RPMs less than 60 is not ideal, as it allows quick bursts of requests, instead of them being evenly spread out over the minute.

If we're sending a request each second, then the max_rate can't be less than 1, or the lib will complain, so 1RPS or 60 RPM is the lowest we can have, which is not good.

This fixes the issue by always having at least 1RPS, but increasing the time gap between requests, if RPM is less than 60.

Copy link
Contributor

@danielnaumau danielnaumau left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@pawel-wroniszewski pawel-wroniszewski left a comment

Choose a reason for hiding this comment

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

I like it

@arnasbr arnasbr merged commit 589f579 into master Sep 25, 2024
5 checks passed
@arnasbr arnasbr deleted the different-rate-limiter-fix branch September 25, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants