Skip to content

Conversation

@beardnick
Copy link
Contributor

@beardnick beardnick commented Nov 16, 2025

Description

Which issue(s) this PR fixes:

Fixes #12482

Notice
  • I have updated the limit-count-redis.lua and limit-count-redis-cluster.lua files to ensure they now support rate-limiting during the log phase.
  • I referred to the limit-conn-redis.lua file as a guide while implementing rate-limiting in the log phase.
  • To ensure a clean testing environment, I added the require("lib.test_redis").flush_all() function to every Redis rate-limiting test case. Without this addition, the tests could fail unpredictably.
  • I have adjusted the expected results in limit-req-redis.t and limit-req-redis-cluster.t test cases. After thorough verification, the correct behavior is [200, 403, 403, 403]. Previously, the results appeared as [403, 403, 403, 403] due to Redis not being properly cleaned beforehand.

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 first)

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Nov 16, 2025
@beardnick beardnick changed the title Feature/ai rate limiting redis support feat: ai rate limiting redis support Nov 16, 2025
@beardnick
Copy link
Contributor Author

@Baoyuantop PTAL

@membphis
Copy link
Member

@beardnick many thx for your contribution, some CI tests are failed, you can fix them


local remaining = res[1]
ttl = res[2]
local function log_phase_incoming_thread(premature, self, key, cost)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code should be placed in the ai-rate-limiting plugin, not in the limit-count plugin itself, because this code is only useful for ai-rate-limiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Similar logic also appears in the limit-conn.

local function leaving_thread(premature, self, key, req_latency)
local conf = self.conf
local red, err = redis.new(conf)
if not red then
return red, err
end
return util.leaving(self, red, key, req_latency)
end
function _M.leaving(self, key, req_latency)
-- log_by_lua can't use cosocket
local ok, err = ngx_timer_at(0, leaving_thread, self, key, req_latency)
if not ok then
core.log.error("failed to create timer: ", err)
return nil, err
end
return ok
end

If I put this logic into ai-rate-limiting plugin, it will make the plugin too complicated. I have to copy a lot of logic from limit-count. Currently, the ai-rate-limiting is just a simple wrapper of limit-count.

Or, do you think I need to rewrite the ai-rate-limiting to something like limit-ai-redis.lua and limit-ai-redis-cluster.lua?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is quite large and hard to review. If you agree, I can split it into 3 separate PRs:

  1. A PR to improve the tests for the rate-limiting plugins.
  2. A PR to add support for the log phase.
  3. A PR to add Redis support to the ai-rate-limiting.

Copy link
Member

@nic-6443 nic-6443 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look like the changes in this PR should be limited to the ai-rate-limiting plugin and minor tweaks to the limit-count plugin. The current PR modifies too much code and test cases for other rate limiting plugins, expanding its scope. We should minimize unnecessary code changes.

=== TEST 21: set route with Redis policy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended to use multi worker testing for Redis related tasks in the new test file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

help request: How to config ai-rate-limiting plugin's counter to redis, as limit-count plugin

4 participants