You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "shreemaan-abhishek (via GitHub)" <gi...@apache.org> on 2026/01/08 02:53:53 UTC
[PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
shreemaan-abhishek opened a new pull request, #12872:
URL: https://github.com/apache/apisix/pull/12872
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes #
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "ChuanFF (via GitHub)" <gi...@apache.org>.
ChuanFF commented on PR #12872:
URL: https://github.com/apache/apisix/pull/12872#issuecomment-3737186403
> when an APISIX node crashes and is unable to perform the "decrement count" operation, this key remains permanently in Redis as there is not expiry configured, causing the user's concurrency quota to be permanently occupied or even the user to be banned.
Setting a TTL for Redis keys does not seem to solve the scenario where a node in an APISIX cluster goes down under high traffic. When the traffic is heavy, APISIX keeps performing Redis operations, which continuously refresh the TTL. As a result, the counter associated with the failed node in Redis cannot be reset through TTL expiration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "ChuanFF (via GitHub)" <gi...@apache.org>.
ChuanFF commented on PR #12872:
URL: https://github.com/apache/apisix/pull/12872#issuecomment-3738318512
@shreemaan-abhishek
**Example:** an APISIX cluster with two nodes, **A** and **B**.
Configuration: `limit_key = limit_conn`, `conf.key_ttl = 10s`, `conf.conn = 10`, `conf.burst = 0`.
- **t = 1s:**
Five requests arrive at node A. Redis `INCR` is called with `5`, resulting in
`limit_conn = 5`, `TTL = 10s`.
- **t = 5s:**
Five requests arrive at node B. Redis `INCR` is called with `5`, resulting in
`limit_conn = 10`, `TTL = 10s`.
- **t = 10s:**
Node A crashes. At this moment, the Redis key remains
`limit_conn = 10` with `TTL = 5s`.
- **t = 14s:**
Five requests finish on node B. Redis is updated, resulting in
`limit_conn = 5`, `TTL = 10s`.
- **t = 20s:**
Six requests finish on node B. Redis is updated to
`limit_conn = 11`, `TTL = 10s`.
Since `11 > conf.conn`, one request is rejected.
Therefore, in scenarios with continuously ongoing requests, it is difficult to rely on TTL expiration to clear the counters of nodes that have gone down.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2711096807
##########
apisix/utils/redis-schema.lua:
##########
@@ -74,8 +74,20 @@ local policy_to_additional_properties = {
},
}
+local limit_conn_redis_cluster_schema = policy_to_additional_properties["redis-cluster"]
+limit_conn_redis_cluster_schema.properties.key_ttl = {
+ type = "integer", default = 3600,
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2680857847
##########
docs/en/latest/plugins/limit-conn.md:
##########
@@ -44,6 +44,7 @@ The `limit-conn` Plugin limits the rate of requests by the number of concurrent
| only_use_default_delay | boolean | False | false | | If false, delay requests proportionally based on how much they exceed the `conn` limit. The delay grows larger as congestion increases. For instance, with `conn` being `5`, `burst` being `3`, and `default_conn_delay` being `1`, 6 concurrent requests would result in a 1-second delay, 7 requests a 2-second delay, 8 requests a 3-second delay, and so on, until the total limit of `conn + burst` is reached, beyond which requests are rejected. If true, use `default_conn_delay` to delay all excessive requests within the `burst` range. Requests beyond `conn + burst` are rejected immediately. For instance, with `conn` being `5`, `burst` being `3`, and `default_conn_delay` being `1`, 6, 7, or 8 concurrent requests are all delayed by exactly 1 second each. |
| key_type | string | False | var | ["var","var_combination"] | The type of key. If the `key_type` is `var`, the `key` is interpreted a variable. If the `key_type` is `var_combination`, the `key` is interpreted as a combination of variables. |
| key | string | False | remote_addr | | The key to count requests by. If the `key_type` is `var`, the `key` is interpreted a variable. The variable does not need to be prefixed by a dollar sign (`$`). If the `key_type` is `var_combination`, the `key` is interpreted as a combination of variables. All variables should be prefixed by dollar signs (`$`). For example, to configure the `key` to use a combination of two request headers `custom-a` and `custom-b`, the `key` should be configured as `$http_custom_a $http_custom_b`. |
+| key_ttl | integer | False | 60 | | The TTL of the Redis key in seconds. Used when `policy` is `redis` or `redis-cluster`. |
Review Comment:
1. update `limit-req` doc for the parameter
2. add the corresponding zh docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek merged PR #12872:
URL: https://github.com/apache/apisix/pull/12872
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2715579601
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -18,36 +18,66 @@
local assert = assert
local math = require "math"
local floor = math.floor
+local ngx = ngx
+local ngx_time = ngx.time
+local uuid = require("resty.jit-uuid")
+
local _M = {version = 0.3}
+local redis_incoming_script = [[
+ local key = KEYS[1]
+ local limit = tonumber(ARGV[1])
+ local ttl = tonumber(ARGV[2])
+ local now = tonumber(ARGV[3])
+ local req_id = ARGV[4]
+
+ redis.call('ZREMRANGEBYSCORE', key, 0, now)
+
+ local count = redis.call('ZCARD', key)
+ if count >= limit then
+ return {0, count}
+ end
+
+ redis.call('ZADD', key, now + ttl, req_id)
+ redis.call('EXPIRE', key, ttl)
+ return {1, count + 1}
+]]
function _M.incoming(self, red, key, commit)
local max = self.max
self.committed = false
+ local raw_key = key
key = "limit_conn" .. ":" .. key
- local conn, err
+ local conn
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
+ local req_id = ngx.ctx.request_id or uuid.generate_v4()
+ if not ngx.ctx.limit_conn_req_ids then
+ ngx.ctx.limit_conn_req_ids = {}
+ end
+ ngx.ctx.limit_conn_req_ids[raw_key] = req_id
+
+ local now = ngx_time()
+ local res, err = red:eval(redis_incoming_script, 1, key,
Review Comment:
you can take a look at this example, which is better performance
we can cache the `sha1` of Lua script, then we call `evalsha`
```lua
local redis = require "resty.redis"
local red = redis:new()
-- 1. Define your fixed script and pre-calculate its SHA1 locally
-- You can use an online SHA1 tool or calculate it via code
local script_content = "return redis.call('GET', KEYS[1])"
local script_sha1 = "6bce267422d7793d3e0034d7c99602029c5737e6"
red:set_timeout(1000) -- 1 sec
local ok, err = red:connect("127.0.0.1", 6379)
if not ok then
ngx.log(ngx.ERR, "failed to connect: ", err)
return
end
--- Function to execute script with EVALSHA optimization
local function optimized_eval(red, sha1, script, num_keys, ...)
-- Try EVALSHA first
local res, err = red:evalsha(sha1, num_keys, ...)
-- If the script is missing in Redis (NOSCRIPT error)
if not res and err and string.find(err, "NOSCRIPT") then
ngx.log(ngx.INFO, "Script not found in cache, loading now...")
-- Load the script to get the SHA1 (or verify it)
local new_sha, load_err = red:script("LOAD", script)
if not new_sha then
return nil, "failed to load script: " .. (load_err or "")
end
-- Retry EVALSHA with the loaded script
return red:evalsha(new_sha, num_keys, ...)
end
return res, err
end
-- 2. Call the function
local res, err = optimized_eval(red, script_sha1, script_content, 1, "my_key")
if not res then
ngx.say("Error: ", err)
else
ngx.say("Result: ", res)
end
-- Put it back in the connection pool
red:set_keepalive(10000, 100)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2716495506
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -18,36 +18,66 @@
local assert = assert
local math = require "math"
local floor = math.floor
+local ngx = ngx
+local ngx_time = ngx.time
+local uuid = require("resty.jit-uuid")
+
local _M = {version = 0.3}
+local redis_incoming_script = [[
+ local key = KEYS[1]
+ local limit = tonumber(ARGV[1])
+ local ttl = tonumber(ARGV[2])
+ local now = tonumber(ARGV[3])
+ local req_id = ARGV[4]
+
+ redis.call('ZREMRANGEBYSCORE', key, 0, now)
+
+ local count = redis.call('ZCARD', key)
+ if count >= limit then
+ return {0, count}
+ end
+
+ redis.call('ZADD', key, now + ttl, req_id)
+ redis.call('EXPIRE', key, ttl)
+ return {1, count + 1}
+]]
function _M.incoming(self, red, key, commit)
local max = self.max
self.committed = false
+ local raw_key = key
key = "limit_conn" .. ":" .. key
- local conn, err
+ local conn
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
+ local req_id = ngx.ctx.request_id or uuid.generate_v4()
+ if not ngx.ctx.limit_conn_req_ids then
+ ngx.ctx.limit_conn_req_ids = {}
+ end
+ ngx.ctx.limit_conn_req_ids[raw_key] = req_id
+
+ local now = ngx_time()
+ local res, err = red:eval(redis_incoming_script, 1, key,
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2674735702
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -68,6 +68,10 @@ function _M.incoming(self, red, key, commit)
if not ok then
return nil, err
end
+
+ local ttl = math.ceil(self.burst / self.rate) + 1
+ red:expire(excess_key, ttl)
+ red:expire(last_key, ttl)
Review Comment:
I think fiexed one second is enough for limit-req
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #12872:
URL: https://github.com/apache/apisix/pull/12872#issuecomment-3776357460
@ChuanFF I have updated the redis script to ensure each redis key has a ttl assigned to it. So the issue that you described should not occur.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2727073505
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -18,36 +18,76 @@
local assert = assert
local math = require "math"
local floor = math.floor
+local ngx = ngx
+local ngx_time = ngx.time
+local uuid = require("resty.jit-uuid")
+local core = require("apisix.core")
+local hex_encode = require("resty.string").to_hex
+
local _M = {version = 0.3}
+local redis_incoming_script = core.string.compress_script([=[
+ local key = KEYS[1]
+ local limit = tonumber(ARGV[1])
+ local ttl = tonumber(ARGV[2])
+ local now = tonumber(ARGV[3])
+ local req_id = ARGV[4]
+
+ redis.call('ZREMRANGEBYSCORE', key, 0, now)
+
+ local count = redis.call('ZCARD', key)
+ if count >= limit then
+ return {0, count}
+ end
+
+ redis.call('ZADD', key, now + ttl, req_id)
+ redis.call('EXPIRE', key, ttl)
+ return {1, count + 1}
+]=])
+local redis_incoming_script_sha = hex_encode(ngx.sha1_bin(redis_incoming_script))
function _M.incoming(self, red, key, commit)
local max = self.max
self.committed = false
+ local raw_key = key
key = "limit_conn" .. ":" .. key
- local conn, err
+ local conn
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
+ local req_id = ngx.ctx.request_id or uuid.generate_v4()
+ if not ngx.ctx.limit_conn_req_ids then
+ ngx.ctx.limit_conn_req_ids = {}
+ end
+ ngx.ctx.limit_conn_req_ids[raw_key] = req_id
+
+ local now = ngx_time()
+ local res, err = red:evalsha(redis_incoming_script_sha, 1, key,
Review Comment:
got it, I have fixed this too now.
FYI, `lua-resty-redis-cluster` didn't work with `evalsha` so I ensured a conditional usage of plain redis eval by rediscluster policy and evalsha usage by `redis` policy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2680859549
##########
apisix/utils/redis-schema.lua:
##########
@@ -78,4 +78,14 @@ local _M = {
schema = policy_to_additional_properties
}
+
+function _M.ttl_policy_schema(kind, ttl)
+ local schema = policy_to_additional_properties[kind]
Review Comment:
The ttl_policy_schema function modifies a shared table, is this safe?
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -28,9 +28,20 @@ function _M.incoming(self, red, key, commit)
local conn, err
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
- return nil, err
+ if self.conf.key_ttl then
+ red:init_pipeline()
Review Comment:
The PR introduces two different methods for setting a TTL:
For maintainability and predictability, we should use a single, consistent approach across all plugins that share a common logic. The SET ... EX approach is generally preferred for its atomicity and efficiency. Why was a different approach chosen for limit-conn?
•limit-conn: Uses a Redis pipeline to issue separate INCRBY and EXPIRE commands.
•limit-req: Uses the atomic SET ... EX command.
##########
t/plugin/limit-conn-redis-cluster-ttl.t:
##########
@@ -0,0 +1,334 @@
+#
Review Comment:
Need to add a scenario where a key's TTL expires. The test should verify that the counter is effectively gone and that a new request can proceed without being blocked by a stale counter.
•Example: Set a short TTL (e.g., 2s), make a request, wait for 3s, and then confirm that a subsequent request does not inherit the old counter value.
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -57,14 +57,16 @@ function _M.incoming(self, red, key, commit)
end
if commit then
- local ok
- local err
- ok, err = red:set(excess_key, excess)
+ -- limit-req does rate limiting per second, so we set the ttl to 2 seconds
+ local ttl = 2
Review Comment:
The TTL for limit-req is hardcoded to 2 seconds. This feels arbitrary and lacks flexibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2681071382
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -28,9 +28,20 @@ function _M.incoming(self, red, key, commit)
local conn, err
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
- return nil, err
+ if self.conf.key_ttl then
+ red:init_pipeline()
Review Comment:
> I think using the pipeline is a bit too complicated.
why? Even if it is complicated, it should not stop us from implementing measures for performance optimisation right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2719225572
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -18,36 +18,76 @@
local assert = assert
local math = require "math"
local floor = math.floor
+local ngx = ngx
+local ngx_time = ngx.time
+local uuid = require("resty.jit-uuid")
+local core = require("apisix.core")
+local hex_encode = require("resty.string").to_hex
+
local _M = {version = 0.3}
+local redis_incoming_script = core.string.compress_script([=[
+ local key = KEYS[1]
+ local limit = tonumber(ARGV[1])
+ local ttl = tonumber(ARGV[2])
+ local now = tonumber(ARGV[3])
+ local req_id = ARGV[4]
+
+ redis.call('ZREMRANGEBYSCORE', key, 0, now)
+
+ local count = redis.call('ZCARD', key)
+ if count >= limit then
+ return {0, count}
+ end
+
+ redis.call('ZADD', key, now + ttl, req_id)
+ redis.call('EXPIRE', key, ttl)
+ return {1, count + 1}
+]=])
+local redis_incoming_script_sha = hex_encode(ngx.sha1_bin(redis_incoming_script))
function _M.incoming(self, red, key, commit)
local max = self.max
self.committed = false
+ local raw_key = key
key = "limit_conn" .. ":" .. key
- local conn, err
+ local conn
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
+ local req_id = ngx.ctx.request_id or uuid.generate_v4()
+ if not ngx.ctx.limit_conn_req_ids then
+ ngx.ctx.limit_conn_req_ids = {}
+ end
+ ngx.ctx.limit_conn_req_ids[raw_key] = req_id
+
+ local now = ngx_time()
+ local res, err = red:evalsha(redis_incoming_script_sha, 1, key,
Review Comment:
it is more safer than we generate it by `ngx api`
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -18,36 +18,76 @@
local assert = assert
local math = require "math"
local floor = math.floor
+local ngx = ngx
+local ngx_time = ngx.time
+local uuid = require("resty.jit-uuid")
+local core = require("apisix.core")
+local hex_encode = require("resty.string").to_hex
+
local _M = {version = 0.3}
+local redis_incoming_script = core.string.compress_script([=[
+ local key = KEYS[1]
+ local limit = tonumber(ARGV[1])
+ local ttl = tonumber(ARGV[2])
+ local now = tonumber(ARGV[3])
+ local req_id = ARGV[4]
+
+ redis.call('ZREMRANGEBYSCORE', key, 0, now)
+
+ local count = redis.call('ZCARD', key)
+ if count >= limit then
+ return {0, count}
+ end
+
+ redis.call('ZADD', key, now + ttl, req_id)
+ redis.call('EXPIRE', key, ttl)
+ return {1, count + 1}
+]=])
+local redis_incoming_script_sha = hex_encode(ngx.sha1_bin(redis_incoming_script))
function _M.incoming(self, red, key, commit)
local max = self.max
self.committed = false
+ local raw_key = key
key = "limit_conn" .. ":" .. key
- local conn, err
+ local conn
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
+ local req_id = ngx.ctx.request_id or uuid.generate_v4()
+ if not ngx.ctx.limit_conn_req_ids then
+ ngx.ctx.limit_conn_req_ids = {}
+ end
+ ngx.ctx.limit_conn_req_ids[raw_key] = req_id
+
+ local now = ngx_time()
+ local res, err = red:evalsha(redis_incoming_script_sha, 1, key,
Review Comment:
another example, gen the sha1 by `redis:script`:
```lua
local function fetch_latest_redis_script_sha1()
if redis_incoming_script_sha then
return redis_incoming_script_sha
end
local sha1, load_err = red:script("LOAD", lua_script)
if load_err then
return nil, load_err
end
redis_incoming_script_sha = sha1
return redis_incoming_script_sha
end
local sha1, err = fetch_latest_redis_script_sha1()
if not sha1 then
ngx.log(ngx.ERR, "Failed to load Lua script into Redis: ", err)
return nil, err
end
local res, err = red:evalsha(sha1, ...)
if err and core.string.has_prefix(err, "NOSCRIPT") then
redis_incoming_script_sha = nil
end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2711089258
##########
docs/en/latest/plugins/limit-conn.md:
##########
@@ -44,6 +44,7 @@ The `limit-conn` Plugin limits the rate of requests by the number of concurrent
| only_use_default_delay | boolean | False | false | | If false, delay requests proportionally based on how much they exceed the `conn` limit. The delay grows larger as congestion increases. For instance, with `conn` being `5`, `burst` being `3`, and `default_conn_delay` being `1`, 6 concurrent requests would result in a 1-second delay, 7 requests a 2-second delay, 8 requests a 3-second delay, and so on, until the total limit of `conn + burst` is reached, beyond which requests are rejected. If true, use `default_conn_delay` to delay all excessive requests within the `burst` range. Requests beyond `conn + burst` are rejected immediately. For instance, with `conn` being `5`, `burst` being `3`, and `default_conn_delay` being `1`, 6, 7, or 8 concurrent requests are all delayed by exactly 1 second each. |
| key_type | string | False | var | ["var","var_combination"] | The type of key. If the `key_type` is `var`, the `key` is interpreted a variable. If the `key_type` is `var_combination`, the `key` is interpreted as a combination of variables. |
| key | string | False | remote_addr | | The key to count requests by. If the `key_type` is `var`, the `key` is interpreted a variable. The variable does not need to be prefixed by a dollar sign (`$`). If the `key_type` is `var_combination`, the `key` is interpreted as a combination of variables. All variables should be prefixed by dollar signs (`$`). For example, to configure the `key` to use a combination of two request headers `custom-a` and `custom-b`, the `key` should be configured as `$http_custom_a $http_custom_b`. |
+| key_ttl | integer | False | 60 | | The TTL of the Redis key in seconds. Used when `policy` is `redis` or `redis-cluster`. |
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2674735702
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -68,6 +68,10 @@ function _M.incoming(self, red, key, commit)
if not ok then
return nil, err
end
+
+ local ttl = math.ceil(self.burst / self.rate) + 1
+ red:expire(excess_key, ttl)
+ red:expire(last_key, ttl)
Review Comment:
I think one second is enough for limit-req
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2680869840
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -64,7 +75,20 @@ function _M.leaving(self, red, key, req_latency)
assert(key)
key = "limit_conn" .. ":" .. key
- local conn, err = red:incrby(key, -1)
+ local conn, err
+ if self.conf.key_ttl then
+ red:init_pipeline()
+ red:incrby(key, -1)
+ red:expire(key, self.conf.key_ttl)
+ local res, err = red:commit_pipeline()
+ if not res then
+ return nil, err
Review Comment:
The return statement has inconsistent indentation - it uses 5 spaces (leading space before 4-space indent) while the rest of the code uses 4-space indentation. This should be corrected to use 4 spaces for consistency with the rest of the file.
```suggestion
return nil, err
```
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -57,14 +57,16 @@ function _M.incoming(self, red, key, commit)
end
if commit then
- local ok
- local err
- ok, err = red:set(excess_key, excess)
+ -- limit-req does rate limiting per second, so we set the ttl to 2 seconds
+ local ttl = 2
Review Comment:
The limit-req plugin uses a hardcoded TTL of 2 seconds for Redis keys, while the limit-conn plugin has a configurable key_ttl parameter with a default of 60 seconds. This inconsistency means:
1. Users cannot configure the TTL for limit-req Redis keys
2. The 2-second TTL may be too short for high-burst scenarios where requests are queued
Consider making the TTL configurable for limit-req as well, similar to limit-conn. The TTL could be calculated as ceil(burst/rate) + a safety margin to ensure keys persist long enough for delayed requests to be processed.
```suggestion
-- limit-req does rate limiting per second. Allow configurable TTL via self.key_ttl,
-- defaulting to 2 seconds to preserve existing behavior if not set.
local ttl = self.key_ttl or 2
```
##########
t/plugin/limit-req-redis-ttl.t:
##########
@@ -0,0 +1,113 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+ my $config = $block->config // <<_EOC_;
+ location /check {
+ content_by_lua_block {
+ local redis = require "resty.redis"
+ local red = redis:new()
+ red:connect("127.0.0.1", 6379)
+ red:flushall()
+
+
+ -- make a request to /access
+ local httpc = require("resty.http").new()
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/access"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET"
+ })
+
+ if not res then
+ ngx.say("failed to request: ", err)
+ return
+ end
+
+ -- key format might include conf_type and conf_version, e.g. limit_req:127.0.0.1route99excess
+ local keys, err = red:keys("limit_req:limit_req_ttl_test_127.0.0.1*")
+ if not keys or #keys == 0 then
+ ngx.say("no keys found")
+ return
+ end
+
+ local ttl = red:ttl(keys[1])
+ if ttl > 0 and ttl <= 11 then
Review Comment:
The test expects a TTL of up to 11 seconds (based on the formula ceil(burst/rate) + 1 = ceil(10/1) + 1 = 11), but the implementation in apisix/plugins/limit-req/util.lua sets a hardcoded TTL of 2 seconds. This test will pass since 2 is within the range (0, 11], but the test is validating against the wrong expected value.
The test should check for TTL <= 2 to match the actual implementation, or the implementation should be updated to use a calculated TTL based on burst and rate parameters.
```suggestion
if ttl > 0 and ttl <= 2 then
```
##########
t/plugin/limit-req-redis-cluster-ttl.t:
##########
@@ -0,0 +1,168 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+ my $config = $block->config // <<_EOC_;
+ location /check {
+ content_by_lua_block {
+ local redis = require "resty.redis"
+ local redis_cluster = require "resty.rediscluster"
+ local serv_list = {
+ { ip = "127.0.0.1", port = 5000 },
+ { ip = "127.0.0.1", port = 5001 },
+ { ip = "127.0.0.1", port = 5002 },
+ { ip = "127.0.0.1", port = 5003 },
+ { ip = "127.0.0.1", port = 5004 },
+ { ip = "127.0.0.1", port = 5005 },
+ }
+ local config = {
+ name = "test-cluster",
+ serv_list = serv_list,
+ keepalive_timeout = 60000,
+ keepalive_cons = 1000,
+ connect_timeout = 1000,
+ socket_timeout = 1000,
+ dict_name = "plugin-limit-req-redis-cluster-slot-lock",
+ }
+
+ -- Flush all keys in the cluster
+ for _, node in ipairs(serv_list) do
+ local red = redis:new()
+ red:set_timeout(1000)
+ local ok, err = red:connect(node.ip, node.port)
+ if ok then
+ red:flushall()
+ red:close()
+ end
+ end
+
+ local red = redis_cluster:new(config)
+
+ -- make a request to /access
+ local httpc = require("resty.http").new()
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/access"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET"
+ })
+
+ if not res then
+ ngx.say("failed to request: ", err)
+ return
+ end
+
+ local found_key
+ for _, node in ipairs(serv_list) do
+ local red_node = redis:new()
+ red_node:set_timeout(1000)
+ local ok, err = red_node:connect(node.ip, node.port)
+ if ok then
+ local keys, err = red_node:keys("limit_req:limit_req_ttl_test_*")
+ if keys and #keys > 0 then
+ found_key = keys[1]
+ red_node:close()
+ break
+ end
+ red_node:close()
+ end
+ end
+
+ if not found_key then
+ ngx.say("no keys found")
+ return
+ end
+
+ local ttl, err = red:ttl(found_key)
+
+ if not ttl or ttl == -2 then
+ ngx.say("no keys found")
+ return
+ end
+
+ -- for rate=1, burst=10 -> ttl = ceil(10/1)+1 = 11.
+ if ttl > 0 and ttl <= 11 then
Review Comment:
The comment indicates that the TTL calculation formula is ceil(burst/rate) + 1, which would equal 11 seconds for rate=1 and burst=10. However, this formula and comment appear to be copied from limit-req tests but are actually checking a hardcoded 2-second TTL in the implementation. This comment is misleading and should be updated to reflect the actual TTL value of 2 seconds that is set in the implementation.
```suggestion
-- ttl is expected to be 2 seconds as set by the current limit-req implementation.
if ttl == 2 then
```
##########
apisix/utils/redis-schema.lua:
##########
@@ -78,4 +78,14 @@ local _M = {
schema = policy_to_additional_properties
}
+
+function _M.ttl_policy_schema(kind, ttl)
+ local schema = policy_to_additional_properties[kind]
+ schema.properties.key_ttl = {
+ type = "integer", default = ttl,
+ }
+ return schema
+end
Review Comment:
This function mutates the original schema object from policy_to_additional_properties by directly adding the key_ttl property. This means that after the first call, the schema will be modified permanently for all subsequent uses. This could lead to unexpected behavior if the schema is reused elsewhere.
Consider creating a deep copy of the schema before adding the key_ttl property to avoid mutating the shared schema object.
##########
t/plugin/limit-req-redis-cluster-ttl.t:
##########
@@ -0,0 +1,168 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+ my $config = $block->config // <<_EOC_;
+ location /check {
+ content_by_lua_block {
+ local redis = require "resty.redis"
+ local redis_cluster = require "resty.rediscluster"
+ local serv_list = {
+ { ip = "127.0.0.1", port = 5000 },
+ { ip = "127.0.0.1", port = 5001 },
+ { ip = "127.0.0.1", port = 5002 },
+ { ip = "127.0.0.1", port = 5003 },
+ { ip = "127.0.0.1", port = 5004 },
+ { ip = "127.0.0.1", port = 5005 },
+ }
+ local config = {
+ name = "test-cluster",
+ serv_list = serv_list,
+ keepalive_timeout = 60000,
+ keepalive_cons = 1000,
+ connect_timeout = 1000,
+ socket_timeout = 1000,
+ dict_name = "plugin-limit-req-redis-cluster-slot-lock",
+ }
+
+ -- Flush all keys in the cluster
+ for _, node in ipairs(serv_list) do
+ local red = redis:new()
+ red:set_timeout(1000)
+ local ok, err = red:connect(node.ip, node.port)
+ if ok then
+ red:flushall()
+ red:close()
+ end
+ end
+
+ local red = redis_cluster:new(config)
+
+ -- make a request to /access
+ local httpc = require("resty.http").new()
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/access"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET"
+ })
+
+ if not res then
+ ngx.say("failed to request: ", err)
+ return
+ end
+
+ local found_key
+ for _, node in ipairs(serv_list) do
+ local red_node = redis:new()
+ red_node:set_timeout(1000)
+ local ok, err = red_node:connect(node.ip, node.port)
+ if ok then
+ local keys, err = red_node:keys("limit_req:limit_req_ttl_test_*")
+ if keys and #keys > 0 then
+ found_key = keys[1]
+ red_node:close()
+ break
+ end
+ red_node:close()
+ end
+ end
+
+ if not found_key then
+ ngx.say("no keys found")
+ return
+ end
+
+ local ttl, err = red:ttl(found_key)
+
+ if not ttl or ttl == -2 then
+ ngx.say("no keys found")
+ return
+ end
+
+ -- for rate=1, burst=10 -> ttl = ceil(10/1)+1 = 11.
+ if ttl > 0 and ttl <= 11 then
Review Comment:
Similar to the redis test, this test expects a TTL of up to 11 seconds based on the formula ceil(burst/rate) + 1, but the implementation uses a hardcoded 2-second TTL. The test should check for TTL <= 2 to match the actual implementation, or the implementation should use a calculated TTL.
```suggestion
-- implementation currently uses a fixed TTL of 2 seconds
if ttl > 0 and ttl <= 2 then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2680889048
##########
apisix/utils/redis-schema.lua:
##########
@@ -78,4 +78,14 @@ local _M = {
schema = policy_to_additional_properties
}
+
+function _M.ttl_policy_schema(kind, ttl)
+ local schema = policy_to_additional_properties[kind]
Review Comment:
No it is not. Good catch. Let me fix this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2681591031
##########
apisix/utils/redis-schema.lua:
##########
@@ -74,8 +74,20 @@ local policy_to_additional_properties = {
},
}
+local limit_conn_redis_cluster_schema = policy_to_additional_properties["redis-cluster"]
+limit_conn_redis_cluster_schema.properties.key_ttl = {
+ type = "integer", default = 60,
Review Comment:
I think the default value can be `3600`, means 1hour
it should be enough
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2681554808
##########
t/plugin/limit-conn-redis-cluster-ttl.t:
##########
@@ -0,0 +1,334 @@
+#
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2710848953
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -18,36 +18,89 @@
local assert = assert
local math = require "math"
local floor = math.floor
+local ngx = ngx
+local ngx_time = ngx.time
+local tonumber = tonumber
+local uuid = require("resty.jit-uuid")
+local core = require("apisix.core")
+
local _M = {version = 0.3}
+local redis_incoming_script = [[
+ local key = KEYS[1]
+ local limit = tonumber(ARGV[1])
+ local ttl = tonumber(ARGV[2])
+ local now = tonumber(ARGV[3])
+ local req_id = ARGV[4]
+
+ redis.call('ZREMRANGEBYSCORE', key, 0, now)
+
+ local count = redis.call('ZCARD', key)
+ if count >= limit then
+ return {0, count}
+ end
+
+ redis.call('ZADD', key, now + ttl, req_id)
+ redis.call('EXPIRE', key, ttl)
+ return {1, count + 1}
+]]
function _M.incoming(self, red, key, commit)
local max = self.max
self.committed = false
+ local raw_key = key
key = "limit_conn" .. ":" .. key
local conn, err
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
- return nil, err
- end
+ if self.conf.key_ttl then
+ local req_id = ngx.ctx.request_id or uuid.generate_v4()
+ if not ngx.ctx.limit_conn_req_ids then
+ ngx.ctx.limit_conn_req_ids = {}
+ end
+ ngx.ctx.limit_conn_req_ids[raw_key] = req_id
Review Comment:
`limit_conn_req_ids` needs to use an array to record multiple request ids because the limit-conn plugin may be executed multiple times in the same request, ref: https://github.com/apache/apisix/pull/1994
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2710848953
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -18,36 +18,89 @@
local assert = assert
local math = require "math"
local floor = math.floor
+local ngx = ngx
+local ngx_time = ngx.time
+local tonumber = tonumber
+local uuid = require("resty.jit-uuid")
+local core = require("apisix.core")
+
local _M = {version = 0.3}
+local redis_incoming_script = [[
+ local key = KEYS[1]
+ local limit = tonumber(ARGV[1])
+ local ttl = tonumber(ARGV[2])
+ local now = tonumber(ARGV[3])
+ local req_id = ARGV[4]
+
+ redis.call('ZREMRANGEBYSCORE', key, 0, now)
+
+ local count = redis.call('ZCARD', key)
+ if count >= limit then
+ return {0, count}
+ end
+
+ redis.call('ZADD', key, now + ttl, req_id)
+ redis.call('EXPIRE', key, ttl)
+ return {1, count + 1}
+]]
function _M.incoming(self, red, key, commit)
local max = self.max
self.committed = false
+ local raw_key = key
key = "limit_conn" .. ":" .. key
local conn, err
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
- return nil, err
- end
+ if self.conf.key_ttl then
+ local req_id = ngx.ctx.request_id or uuid.generate_v4()
+ if not ngx.ctx.limit_conn_req_ids then
+ ngx.ctx.limit_conn_req_ids = {}
+ end
+ ngx.ctx.limit_conn_req_ids[raw_key] = req_id
Review Comment:
`limit_conn_req_ids` needs to use an array to record multiple request ids because the limit-conn plugin may be executed multiple times in the same request, ref: https://github.com/apache/apisix/pull/1994
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2674736474
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -68,6 +68,10 @@ function _M.incoming(self, red, key, commit)
if not ok then
return nil, err
end
+
+ local ttl = math.ceil(self.burst / self.rate) + 1
+ red:expire(excess_key, ttl)
+ red:expire(last_key, ttl)
Review Comment:
and can we set ttl by `redis:set` api directly? otherwise we introduce another two redis calls per request now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2680886189
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -28,9 +28,20 @@ function _M.incoming(self, red, key, commit)
local conn, err
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
- return nil, err
+ if self.conf.key_ttl then
+ red:init_pipeline()
Review Comment:
Unlike `limit-req`, `limit-conn` uses `incrby`, and `incrby` does not support passing an `expiry` parameter.
Using redis pipeline to perform incrby and expire over a single call instead of two different incrby and expire calls is more performant.
> •limit-req: Uses the atomic SET ... EX command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2710860784
##########
apisix/utils/redis-schema.lua:
##########
@@ -74,8 +74,20 @@ local policy_to_additional_properties = {
},
}
+local limit_conn_redis_cluster_schema = policy_to_additional_properties["redis-cluster"]
+limit_conn_redis_cluster_schema.properties.key_ttl = {
+ type = "integer", default = 3600,
Review Comment:
If a default value is set for ttl, then the check in the program when `self.conf.key_ttl` is nil becomes meaningless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(limit-conn): implement configurable redis key expiry [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2681613662
##########
apisix/utils/redis-schema.lua:
##########
@@ -74,8 +74,20 @@ local policy_to_additional_properties = {
},
}
+local limit_conn_redis_cluster_schema = policy_to_additional_properties["redis-cluster"]
+limit_conn_redis_cluster_schema.properties.key_ttl = {
+ type = "integer", default = 60,
Review Comment:
makes sense, I have set the default ttl to be 3600.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2675070894
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -68,6 +68,10 @@ function _M.incoming(self, red, key, commit)
if not ok then
return nil, err
end
+
+ local ttl = math.ceil(self.burst / self.rate) + 1
+ red:expire(excess_key, ttl)
+ red:expire(last_key, ttl)
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2680934052
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -28,9 +28,20 @@ function _M.incoming(self, red, key, commit)
local conn, err
if commit then
- conn, err = red:incrby(key, 1)
- if not conn then
- return nil, err
+ if self.conf.key_ttl then
+ red:init_pipeline()
Review Comment:
I think using the pipeline is a bit too complicated. For limit-req, I support you continuing to use the additional `expire` call. In fact, my previous suggestion was only for limit-conn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add ttl to redis counters in redis policy rate limiters [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #12872:
URL: https://github.com/apache/apisix/pull/12872#issuecomment-3737718459
> As a result, the counter associated with the failed node in Redis cannot be reset through TTL expiration.
I am not sure if I fully understand what you are trying to convey. The counter key will be same for all apisix instances in a cluster. Am I missing something 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org