nic-6443 commented on code in PR #13363:
URL: https://github.com/apache/apisix/pull/13363#discussion_r3225321261
##########
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:
This test file already owns the Redis instance exclusively — no other test
files are running concurrently in the same CI job. `FLUSHALL` ensures a clean
state for the NOSCRIPT fallback test. Using `FLUSHDB` would work too, but
`FLUSHALL` is simpler and equally safe here.
##########
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:
Same reasoning as the gateway PR — these are basic Redis commands in a test
helper. If they fail, TEST 6c will fail with a clear signal. Adding error
handling would be noise.
##########
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 fallback is only 4 lines (evalsha + if NOSCRIPT + warn + eval).
Extracting a shared helper would add more complexity (function signature,
require) than it saves. The two files also live in different directories and
are loaded independently. Keeping the pattern inline is consistent with how
`limit-conn/util.lua` does it.
--
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]
