You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/07/29 17:37:35 UTC
[GitHub] [incubator-apisix] shuaijinchao opened a new pull request #1928: bugfix: fix limit-count plugin redis.ttl error.
shuaijinchao opened a new pull request #1928:
URL: https://github.com/apache/incubator-apisix/pull/1928
### What this PR does / why we need it:
FIX #1901
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on a change in pull request #1928: bugfix: fix limit-count plugin redis.ttl error.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1928:
URL: https://github.com/apache/incubator-apisix/pull/1928#discussion_r462273477
##########
File path: apisix/plugins/limit-count/limit-count-redis.lua
##########
@@ -71,7 +71,8 @@ function _M.incoming(self, key)
local remaining
key = self.plugin_name .. tostring(key)
- local ret = red:ttl(key)
+ -- todo: test case
+ local ret = red:ttl(key) or -2
Review comment:
I think we need to cache the `err`, eg: `local ret, err = red:(...)`
And we should write this error message to log, set the right response like `500`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] shuaijinchao commented on a change in pull request #1928: bugfix: fix limit-count plugin redis.ttl error.
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on a change in pull request #1928:
URL: https://github.com/apache/incubator-apisix/pull/1928#discussion_r462695885
##########
File path: apisix/plugins/limit-count/limit-count-redis.lua
##########
@@ -72,7 +72,11 @@ function _M.incoming(self, key)
key = self.plugin_name .. tostring(key)
-- todo: test case
- local ret = red:ttl(key) or -2
+ local ret, err = red:ttl(key)
+ if not ret then
+ return false, "failed to get redis ttl: " .. err
Review comment:
OK, good idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on a change in pull request #1928: bugfix: fix limit-count plugin redis.ttl error.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1928:
URL: https://github.com/apache/incubator-apisix/pull/1928#discussion_r462604190
##########
File path: apisix/plugins/limit-count/limit-count-redis.lua
##########
@@ -72,7 +72,11 @@ function _M.incoming(self, key)
key = self.plugin_name .. tostring(key)
-- todo: test case
- local ret = red:ttl(key) or -2
+ local ret, err = red:ttl(key)
+ if not ret then
+ return false, "failed to get redis ttl: " .. err
Review comment:
we'd better contain the `key` in the error message, it is useful
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] shuaijinchao commented on a change in pull request #1928: bugfix: fix limit-count plugin redis.ttl error.
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on a change in pull request #1928:
URL: https://github.com/apache/incubator-apisix/pull/1928#discussion_r462309855
##########
File path: apisix/plugins/limit-count/limit-count-redis.lua
##########
@@ -71,7 +71,8 @@ function _M.incoming(self, key)
local remaining
key = self.plugin_name .. tostring(key)
- local ret = red:ttl(key)
+ -- todo: test case
+ local ret = red:ttl(key) or -2
Review comment:
OK, fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis merged pull request #1928: bugfix: fix limit-count plugin redis.ttl error.
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #1928:
URL: https://github.com/apache/incubator-apisix/pull/1928
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on pull request #1928: bugfix: fix limit-count plugin redis.ttl error.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1928:
URL: https://github.com/apache/incubator-apisix/pull/1928#issuecomment-666125304
merged, many thx @shuaijinchao
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org