Copilot commented on code in PR #13363:
URL: https://github.com/apache/apisix/pull/13363#discussion_r3225312055
##########
apisix/plugins/limit-count/limit-count-redis.lua:
##########
@@ -65,7 +67,11 @@ function _M.incoming(self, key, cost)
key = self.plugin_name .. tostring(key)
local ttl = 0
- res, err = red:eval(script, 1, key, limit, window, cost or 1)
+ res, err = red:evalsha(script_sha, 1, key, limit, window, cost or 1)
+ if err and core.string.has_prefix(err, "NOSCRIPT") then
+ core.log.warn("redis evalsha failed: ", err, ". Falling back to eval")
+ res, err = red:eval(script, 1, key, limit, window, cost or 1)
+ end
Review Comment:
The `evalsha` + `NOSCRIPT` fallback logic is duplicated in both the
standalone and cluster implementations. Consider extracting a small shared
helper (e.g., `evalsha_with_fallback(red, script, script_sha, ...)`) to keep
behavior and logging consistent and reduce the risk of the two implementations
diverging over time. (Optional, but would improve long-term maintainability.)
##########
t/plugin/limit-count-redis.t:
##########
@@ -195,6 +195,40 @@ GET /hello
+=== TEST 6b: flush redis script cache
+--- config
+ location /t {
+ content_by_lua_block {
+ local redis = require("resty.redis")
+ local red = redis:new()
+ red:set_timeout(1000)
+ local ok, err = red:connect("127.0.0.1", 6379)
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+ red:script("FLUSH")
+ red:flushall()
Review Comment:
`FLUSHALL` clears *all* keys in *all* Redis DBs on that instance, which can
cause cross-test interference if the same Redis is shared (e.g., other test
files/processes) and is unnecessarily disruptive for validating `NOSCRIPT`
behavior. Prefer `FLUSHDB` (or removing the data flush entirely) since only
`SCRIPT FLUSH` is needed to force the `EVALSHA` fallback path.
##########
t/plugin/limit-count-redis.t:
##########
@@ -195,6 +195,40 @@ GET /hello
+=== TEST 6b: flush redis script cache
+--- config
+ location /t {
+ content_by_lua_block {
+ local redis = require("resty.redis")
+ local red = redis:new()
+ red:set_timeout(1000)
+ local ok, err = red:connect("127.0.0.1", 6379)
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+ red:script("FLUSH")
+ red:flushall()
+ red:set_keepalive(10000, 100)
Review Comment:
The test ignores return values from `script(FLUSH)`, `flushall/flushdb`, and
`set_keepalive`. If any of these fail (auth, permissions, Redis not ready), the
test can produce confusing downstream failures. Capture and check `ok, err` for
these calls and emit a clear failure message/early return when they fail.
--
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]
