-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat(ai): add dynamic route cache key #8113
Conversation
apisix/core/ai.lua
Outdated
@@ -71,6 +97,26 @@ function _M.routes_analyze(routes) | |||
else | |||
core.log.info("use ai plane to match route") | |||
router.match = ai_match | |||
|
|||
if get_cache_key_func_def_render == nil then | |||
local template = require("resty.template") |
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.
can we do this template
at top level?
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.
resty.template depends on nginx.location
, but ai.lua would be called in init_by_lua
, where no ngx.location
, so for the top level case, the require
would fail.
apisix/core/ai.lua
Outdated
get_cache_key_func_def_render = template.compile(get_cache_key_func_def) | ||
end | ||
|
||
local str = get_cache_key_func_def_render({route_flags = route_flags}) |
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.
can we move this logic to one function?
then we can check the return value, and do different thing
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.
pls add some test cases to check it works fine
apisix/core/ai.lua
Outdated
local str = get_cache_key_func_def_render({route_flags = route_flags}) | ||
local func, err = loadstring(str) | ||
if func == nil then | ||
return err |
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.
oh my god, this is not a good design.
we should use return false, err
, the error message can be ignore in some case
apisix/core/ai.lua
Outdated
@@ -71,6 +118,12 @@ function _M.routes_analyze(routes) | |||
else | |||
core.log.info("use ai plane to match route") | |||
router.match = ai_match | |||
|
|||
local err = gen_get_cache_key_func(route_flags) |
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.
ditto
apisix/core/ai.lua
Outdated
return false, err | ||
else | ||
local ok | ||
ok, get_cache_key_func = pcall(func) |
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.
Better to use a temp var instead of hijacking a module var to hold the potential err message
apisix/core/ai.lua
Outdated
|
||
local get_cache_key_func_def = [[ | ||
return function(ctx) | ||
return ctx.var.uri |
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.
We can make it a little faster, by using local var = ctx.var
to reduce table lookup.
t/core/ai.t
Outdated
ngx.sleep(1) | ||
|
||
-- enable | ||
do |
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.
Let's exact this common part into a function, instead of repeating it three times.
-- TODO: we need to generate cache key dynamically | ||
local key = ctx.var.uri .. "-" .. ctx.var.method .. "-" .. ctx.var.host | ||
local key = get_cache_key_func(ctx) | ||
core.log.info("route cache key: ", core.log.delay_exec(encode_base64, key)) |
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.
@kingluo after my benchmark, we have to remove this line.
so we have to find a new way of testing.
local var = ctx.var | ||
return var.uri | ||
{% if route_flags["methods"] then %} | ||
.. "\0" .. var.method |
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.
the uri
, host
and method
, they should generally not contain #
Description
add dynamic route cache key
Checklist