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

request help: Will limit-req support nodelay and rate in minute #4382

Closed
nanamikon opened this issue Jun 7, 2021 · 5 comments
Closed

request help: Will limit-req support nodelay and rate in minute #4382

nanamikon opened this issue Jun 7, 2021 · 5 comments

Comments

@nanamikon
Copy link
Contributor

Issue description

Will limit-req support nodelay and rate in minute?
If not, I find nodelay can be implemented by adding condition, but resty.limit.req only support rate in second, how can I implemnt rate in minute?

Environment

Request help without environment information will be ignored or closed.

  • apisix version (cmd: apisix version): 2.5
  • OS (cmd: uname -a): ubuntu
  • OpenResty / Nginx version (cmd: nginx -V or openresty -V):
  • etcd version, if have (cmd: run curl http://127.0.0.1:9090/v1/server_info to get the info from server-info API):
  • apisix-dashboard version, if have:
  • luarocks version, if the issue is about installation (cmd: luarocks --version):
@spacewander
Copy link
Member

spacewander commented Jun 7, 2021

I find nodelay can be implemented by adding condition

PR is welcome!

how can I implemnt rate in minute

Divide the rate by 60 is already enough? IMHO, there is no need to add a field. Since Nginx also does the same way: https://github.com/nginx/nginx/blob/5eadaf69e394c030056e4190d86dae0262f8617c/src/http/modules/ngx_http_limit_req_module.c#L913

@nanamikon
Copy link
Contributor Author

nanamikon commented Jun 7, 2021

In my company, many users set 200 r/m on remote_address or even lower, If it divided by 60, 1r/s or 2r/s will be reached easily, because modern browser support http2, so one user visit one nignx over 1r/s is very common.

Now, we transfer this conf to apisix, some of them reach limit and delay request online。

I think we use nodelay in nginx conf while apisix only support delay cause this happen, because both of them reach the rate limit, but not over burst. I will test it

The rate is specified in requests per second (r/s). If a rate of less than one request per second is desired, it is specified in request per minute (r/m). For example, half-request per second is 30r/m.
ctx->rate = rate * 1000 / scale;

Maybe multiply by 1000 is the key to solve the decimal point ?

@spacewander
Copy link
Member

In my company, many users set 200 r/m on remote_address or even lower, If it divided by 60, 1r/s or 2r/s will be reached easily,

It's how Nginx works too. Note that the r/m is the speed rate, it doesn't mean you can have 200 requests in a minute, it means you can't be faster than 200r/m. Drive a car in 36km/h is equal to 10m/s.

because modern browser support http2, so one user visit one nignx over 1r/s is very common.

In this case we need a good burst and/or use delay.

ctx->rate = rate * 1000 / scale;

resty.limit.req does the same thing, see https://github.com/openresty/lua-resty-limit-traffic/blob/56b9a35a69117ed0e8a2e3b9a0866c426403e1b9/lib/resty/limit/req.lua#L144

@nanamikon
Copy link
Contributor Author

Thanks for reply, after adding nodelay, we solve the online problem

resty.limit.req does the same thing, see https://github.com/openresty/lua-resty-limit-traffic/blob/56b9a35a69117ed0e8a2e3b9a0866c426403e1b9/lib/resty/limit/req.lua#L144

So mayby rate should be float, but I think it is not the key point

In this case we need a good burst and/or use delay.

I want to submit a pr to add nodelay function, a new param as follow

Name Type Requirement Default
nodelay boolean optional false

Is it ok?

@spacewander
Copy link
Member

LGTM

nanamikon pushed a commit to nanamikon/apisix that referenced this issue Jun 8, 2021
nanamikon pushed a commit to nanamikon/apisix that referenced this issue Jun 8, 2021
nanamikon pushed a commit to nanamikon/apisix that referenced this issue Jun 9, 2021
nanamikon pushed a commit to nanamikon/apisix that referenced this issue Jun 10, 2021
nanamikon pushed a commit to nanamikon/apisix that referenced this issue Jun 10, 2021
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

No branches or pull requests

2 participants