Skip to content

Commit

Permalink
fix(plugins) do not call 'redis:select' when not necessary (bis)
Browse files Browse the repository at this point in the history
In #3973 the logic of selectively calling redis:select() is implemented
for increment() but not usage(). This patch applies the same logic
to usage() as well.

See #3973
From #4117
  • Loading branch information
fffonion authored and thibaultcha committed Jan 2, 2019
1 parent 950fc30 commit f916e2e
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 42 deletions.
44 changes: 23 additions & 21 deletions kong/plugins/rate-limiting/policies/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,13 @@ return {
local red = redis:new()

red:set_timeout(conf.redis_timeout)

local ok, err = red:connect(conf.redis_host, conf.redis_port)
-- use a special pool name only if redis_database is set to non-zero
-- otherwise use the default pool name host:port
sock_opts.pool = conf.redis_database and
conf.redis_host .. ":" .. conf.redis_port ..
":" .. conf.redis_database
local ok, err = red:connect(conf.redis_host, conf.redis_port,
sock_opts)
if not ok then
kong.log.err("failed to connect to Redis: ", err)
return nil, err
Expand All @@ -226,27 +231,24 @@ return {
return nil, err
end

if times == 0 and is_present(conf.redis_password) then
local ok, err = red:auth(conf.redis_password)
if not ok then
kong.log.err("failed to connect to Redis: ", err)
return nil, err
if times == 0 then
if is_present(conf.redis_password) then
local ok, err = red:auth(conf.redis_password)
if not ok then
kong.log.err("failed to auth Redis: ", err)
return nil, err
end
end
end

if times ~= 0 or conf.redis_database then
-- The connection pool is shared between multiple instances of this
-- plugin, and instances of the response-ratelimiting plugin.
-- Because there isn't a way for us to know which Redis database a given
-- socket is connected to without a roundtrip, we force the retrieved
-- socket to select the desired database.
-- When the connection is fresh and the database is the default one, we
-- can skip this roundtrip.

local ok, err = red:select(conf.redis_database or 0)
if not ok then
kong.log.err("failed to change Redis database: ", err)
return nil, err
if conf.redis_database ~= 0 then
-- Only call select first time, since we know the connection is shared
-- between instances that use the same redis database

local ok, err = red:select(conf.redis_database)
if not ok then
kong.log.err("failed to change Redis database: ", err)
return nil, err
end
end
end

Expand Down
44 changes: 23 additions & 21 deletions kong/plugins/response-ratelimiting/policies/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,13 @@ return {
usage = function(conf, identifier, name, period, current_timestamp)
local red = redis:new()
red:set_timeout(conf.redis_timeout)

local ok, err = red:connect(conf.redis_host, conf.redis_port)
-- use a special pool name only if redis_database is set to non-zero
-- otherwise use the default pool name host:port
sock_opts.pool = conf.redis_database and
conf.redis_host .. ":" .. conf.redis_port ..
":" .. conf.redis_database
local ok, err = red:connect(conf.redis_host, conf.redis_port,
sock_opts)
if not ok then
kong.log.err("failed to connect to Redis: ", err)
return nil, err
Expand All @@ -221,27 +226,24 @@ return {
return nil, err
end

if times == 0 and is_present(conf.redis_password) then
local ok, err = red:auth(conf.redis_password)
if not ok then
kong.log.err("failed to auth Redis: ", err)
return nil, err
if times == 0 then
if is_present(conf.redis_password) then
local ok, err = red:auth(conf.redis_password)
if not ok then
kong.log.err("failed to auth Redis: ", err)
return nil, err
end
end
end

if times ~= 0 or conf.redis_database then
-- The connection pool is shared between multiple instances of this
-- plugin, and instances of the response-ratelimiting plugin.
-- Because there isn't a way for us to know which Redis database a given
-- socket is connected to without a roundtrip, we force the retrieved
-- socket to select the desired database.
-- When the connection is fresh and the database is the default one, we
-- can skip this roundtrip.

local ok, err = red:select(conf.redis_database or 0)
if not ok then
kong.log.err("failed to change Redis database: ", err)
return nil, err
if conf.redis_database ~= 0 then
-- Only call select first time, since we know the connection is shared
-- between instances that use the same redis database

local ok, err = red:select(conf.redis_database)
if not ok then
kong.log.err("failed to change Redis database: ", err)
return nil, err
end
end
end

Expand Down

0 comments on commit f916e2e

Please sign in to comment.