Skip to content
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.

Sign up for GitHub

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

Jump to bottom

change(limit-count): ensure redis cluster name is set correctly #3910

Merged
merged 15 commits into from
Apr 7, 2021
Merged

change(limit-count): ensure redis cluster name is set correctly #3910

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
85407f7
fix the problem of multiple different redis-cluster data sources, str…
Mar 25, 2021
9cc3be1
set redis cluster name by etcd config
Mar 29, 2021
f7a5436
set redis cluster name by etcd config
Mar 30, 2021
3c1035f
redis_cluster_name is required parameter
Mar 30, 2021
bab152b
fix bad style
han6565 Mar 30, 2021
c554a1b
fix English grammar issues
han6565 Mar 30, 2021
499d6fa
test for redis_cluster_name
Mar 30, 2021
257d1de
update the example
Mar 30, 2021
b5ce549
Merge remote-tracking branch 'upstream/master'
Mar 31, 2021
0aa6ed6
Merge remote-tracking branch 'upstream/master'
Apr 1, 2021
0157968
update the doc
Apr 2, 2021
338d2c4
update the doc
Apr 2, 2021
c77b409
Merge remote-tracking branch 'upstream/master'
Apr 2, 2021
33b65a0
Merge remote-tracking branch 'upstream/master'
Apr 6, 2021
eb88001
update the doc
Apr 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apisix/plugins/limit-count.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ end

function _M.access(conf, ctx)
core.log.info("ver: ", ctx.conf_version)
conf.name = ctx.route_id
local lim, err = core.lrucache.plugin_ctx(lrucache, ctx, conf.policy, create_limit_obj, conf)
if not lim then
core.log.error("failed to fetch limit.count object: ", err)
Expand Down
2 changes: 1 addition & 1 deletion apisix/plugins/limit-count/limit-count-redis-cluster.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ local mt = {

local function new_redis_cluster(conf)
local config = {
name = "apisix-redis-cluster",
name = "apisix-redis-cluster-" .. conf.name,
Copy link
Member

Choose a reason for hiding this comment

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

I am confused by the solution. What is the relation between the password and name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one name Mapping one routes，if all redis_cluster config the same name. When concurrent lrucache can not find the correct redis_cluster in pool.
This is my personal opinion ，maybe it is wrong.😄

Copy link
Member

@spacewander spacewander Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I read through the resty.rediscluster. Look like this library uses a module-level slot_cache, and the key is the config.name.

So it is no doubted that different configurations with the same name will collide.

What about using "apisix-redis-cluster-" .. tostring(conf) directly?
Better to add a comment about this change as future maintainers may not read through the library.

Different routes using the same cluster will still need to share the slot_cache. Would it be better to use crc32(nodes) to generate the suffix?

Sorry for my fickle mind. It seems the library doesn't provide a way to clean up stale cached slots. So using auto-generated name is unsafe. My latest idea is to require user to specify the name of redis cluster. It is a break change but it is reasonable to introduce break change to fix bug.

serv_list = {},
read_timeout = conf.redis_timeout,
auth = conf.redis_password,
Expand Down