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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
