-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: ai rate limiting redis support #12751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: ai rate limiting redis support #12751
Conversation
|
@Baoyuantop PTAL |
|
@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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
apisix/apisix/plugins/limit-conn/limit-conn-redis.lua
Lines 60 to 81 in 7e907a5
| 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?
There was a problem hiding this comment.
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:
- A PR to improve the tests for the rate-limiting plugins.
- A PR to add support for the log phase.
- A PR to add Redis support to the ai-rate-limiting.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
Description
Which issue(s) this PR fixes:
Fixes #12482
Notice
limit-count-redis.luaandlimit-count-redis-cluster.luafiles to ensure they now support rate-limiting during the log phase.limit-conn-redis.luafile as a guide while implementing rate-limiting in the log phase.require("lib.test_redis").flush_all()function to every Redis rate-limiting test case. Without this addition, the tests could fail unpredictably.limit-req-redis.tandlimit-req-redis-cluster.ttest 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