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.

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

fix(plugins) don't call redis:select when not necessary #3973

Merged

Conversation

Projects
None yet
4 participants
@fffonion
Copy link
Contributor

fffonion commented Nov 12, 2018

Summary

In plugin rate-limiting and response-ratelimiting, a redis:select is
called when connection is reused, even though the current configured
redis database is 0, to clear the previously selected database. This
will be problem for some use case like twemproxy where select is
not supported, and sending select call will cause the connection to
be closed.

The patch used a host+port+database based redis connection pool, so that
we always know the current connection is using correct database. The
behaviour of redis:auth only once is not changed.

Full changelog

  • Use database-based redis connection pool to reduce unnecessary select call

Issues resolved

Related to #3293

fix(plugins) don't call redis:select when not necessary
In plugin rate-limiting and response-ratelimiting, a redis:select is
called when connection is reused, even though the current configured
redis database is 0, to clear the previously selected database. This
will be problem when for some use case like twemproxy where select is
not supported, and sending `select` call will cause the connection to
be closed.

The patch used a host+port+database based redis connection pool, so that
we always know the current connection is using correct database.
-- return a special pool name only if redis_database is set to non-zero
-- otherwise use the default pool name host:port
if conf.redis_database ~= 0 then
local key = fmt("%s:%d:%d", conf.redis_port, conf.redis_port, conf.redis_database)

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Nov 13, 2018

Member

The second argument here should be conf.redis_host

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Nov 13, 2018

Member

Additionally, prior art has proven that a simple concat operation could perform as well or better than string.format, we should maybe follow this new rule of thumb.

@@ -136,7 +145,8 @@ return {
increment = function(conf, limits, identifier, current_timestamp, value)
local red = redis:new()
red:set_timeout(conf.redis_timeout)
local ok, err = red:connect(conf.redis_host, conf.redis_port)
local ok, err = red:connect(conf.redis_host, conf.redis_port,
{ pool = get_redis_pool_name(conf) })

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Nov 13, 2018

Member

We could reuse this table (if declared at the top chunk level) and just override its pool property, to save some GC overhead, and some function call overhead as well:

sock_opts.pool = get_redis_pool_name(conf) -- could even be inlined
@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Nov 13, 2018

Nice catch :) Meant to request some changes, but mis-clicked. Additionally, note that the linter is failing in CI.

@fffonion

This comment has been minimized.

Copy link
Contributor Author

fffonion commented Nov 13, 2018

@thibaultcha Thank you for reviewing this, I've addressed the comments and CI is green now. Could you take another look when you got chance?

if not ok then
ngx_log(ngx.ERR, "failed to change Redis database: ", err)
return nil, err
local ok, err = red:select(conf.redis_database or 0)

This comment has been minimized.

Copy link
@p0pr0ck5

p0pr0ck5 Nov 14, 2018

Contributor

probably dont need the or 0 here?

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Nov 15, 2018

LGTM, however we should wait until our Travis CI issues are resolved before merging this.

@fffonion

This comment has been minimized.

Copy link
Contributor Author

fffonion commented Nov 20, 2018

Hi @thibaultcha, do we have an update on the CI issue?

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Nov 21, 2018

@fffonion It should be fine if you rebase this branch on the latest master.

@hishamhm

This comment has been minimized.

Copy link
Member

hishamhm commented Nov 21, 2018

Running green on master!

@hishamhm hishamhm merged commit fc46f23 into master Nov 21, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.

@hishamhm hishamhm deleted the fix/plugins-dont-call-redis-select-when-not-necessary branch Nov 21, 2018

fffonion added a commit that referenced this pull request Dec 18, 2018

fffonion added a commit that referenced this pull request Dec 18, 2018

thibaultcha added a commit that referenced this pull request Jan 2, 2019

fix(plugins) do not call 'redis:select' when not necessary (bis)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.