You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "nic-6443 (via GitHub)" <gi...@apache.org> on 2026/05/12 09:16:38 UTC
[PR] perf(limit-count): use evalsha with NOSCRIPT fallback for Redis script execution [apisix]
nic-6443 opened a new pull request, #13363:
URL: https://github.com/apache/apisix/pull/13363
The limit-count plugin ships the full Lua script text on every Redis call via `red:eval()`. At high RPS this wastes both network bandwidth (~200-500 bytes per call) and Redis CPU (script parsing on every call).
This PR replaces `eval` with `evalsha` + NOSCRIPT fallback in both the standalone Redis and Redis Cluster backends of the limit-count plugin. The script SHA1 is computed client-side at module load time, avoiding the need for `SCRIPT LOAD` (which doesn't route correctly in Redis Cluster). On NOSCRIPT (after Redis restart or hitting a new cluster shard), the code falls back to `eval` which implicitly caches the script for subsequent calls.
This mirrors the pattern already used by the `limit-conn` plugin, but improves on it by supporting Redis Cluster mode (limit-conn currently disables evalsha for cluster).
Changes:
- `apisix/plugins/limit-count/limit-count-redis.lua`: evalsha + NOSCRIPT fallback
- `apisix/plugins/limit-count/limit-count-redis-cluster.lua`: evalsha + NOSCRIPT fallback
- `t/plugin/limit-count-redis.t`: add test for NOSCRIPT fallback recovery
--
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] perf(limit-count): use evalsha with NOSCRIPT fallback for Redis script execution [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(limit-count): use evalsha with NOSCRIPT fallback for Redis script execution [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 merged PR #13363:
URL: https://github.com/apache/apisix/pull/13363
--
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] perf(limit-count): use evalsha with NOSCRIPT fallback for Redis script execution [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org