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

rate_limit: Adds a --rate option for RPS limits #11271

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

Conversation

zwoop
Copy link
Contributor

@zwoop zwoop commented Apr 19, 2024

No description provided.

@zwoop zwoop added the rate_limit rate_limit plugin label Apr 19, 2024
@zwoop zwoop added this to the 10.1.0 milestone Apr 19, 2024
@zwoop zwoop requested review from shukitchan and elsloo April 19, 2024 14:46
@zwoop zwoop self-assigned this Apr 19, 2024
@zwoop zwoop force-pushed the RateLimitRate branch 3 times, most recently from 66cd3f7 to b3d25e7 Compare April 19, 2024 14:53
@zwoop
Copy link
Contributor Author

zwoop commented Apr 19, 2024

Being a token bucket algorithm, which refills tokens every 25ms evenly distributed, it's not 100% accurate. Doing a h2load test, full blast, with ATS configured with --rate=10000, I get

leif@odin ~ » /opt/h3tools/bin/h2load -D 60 -t 24 -n 100000000 --h1  -c 900 `cat /tmp/urls-frigg.txt | xargs` | tail -9
finished in 60.11s, 508939.12 req/s, 200.47MB/s
requests: 30536347 total, 30537247 started, 30536347 done, 609490 succeeded, 29926857 failed, 0 errored, 0 timeout
status codes: 609490 2xx, 0 3xx, 29926857 4xx, 0 5xx

This is 10,158 RPS which are not rejected, which is pretty close to the target of 10k.

@zwoop zwoop force-pushed the RateLimitRate branch 3 times, most recently from ff8a312 to 61d1fa4 Compare April 19, 2024 16:11

#include "limiter.h"

BucketManager gBucketMngr;
Copy link
Contributor

@ywkaras ywkaras Apr 19, 2024

Choose a reason for hiding this comment

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

Why not make all the members of BucketManager static, and delete the default and copy constructors? Why does it have protected members, what class would ever inherit from BucketManager?


}; // End class BucketManager

extern BucketManager gBucketMngr;

// order must align with the above
static const char *types[] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this on the CI builds:

../plugins/experimental/rate_limit/limiter.h:168:20: error: 'suffixes' defined but not used [-Werror=unused-variable]
  168 | static const char *suffixes[] = {
      |                    ^~~~~~~~
../plugins/experimental/rate_limit/limiter.h:152:20: error: 'types' defined but not used [-Werror=unused-variable]
  152 | static const char *types[] = {
      |                    ^~~~~
cc1plus: note: unrecognized command-line option '-Wno-vla-extension' may have been intended to silence earlier diagnostics

@bryancall bryancall self-requested a review April 22, 2024 22:09
@zwoop zwoop force-pushed the RateLimitRate branch 2 times, most recently from 6943114 to 9f3529c Compare April 24, 2024 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rate_limit rate_limit plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants