[GitHub] [apisix] ychdesign commented on a change in pull request #3991: fix: ensure automic operation in limit-count plugin

[GitBox <gi...@apache.org>]

ychdesign commented on a change in pull request #3991:
URL: https://github.com/apache/apisix/pull/3991#discussion_r608281984



##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -72,62 +71,28 @@ function _M.new(plugin_name, limit, window, conf)
     return setmetatable(self, mt)
 end
 
+local script = "if redis.call('ttl',KEYS[1]) < 0 then "
+    .. "redis.call('set',KEYS[1],ARGV[1]-1,'EX',ARGV[2]) "
+    .. "return ARGV[1]-1 "
+    .. "end "
+    .. "return redis.call('incrby',KEYS[1],-1)"
 
 function _M.incoming(self, key)
     local red = self.red_cli
     local limit = self.limit
     local window = self.window
-    local remaining
     key = self.plugin_name .. tostring(key)
 
-    local ret, err = red:ttl(key)
-    if not ret then
-        return false, "failed to get redis `" .. key .."` ttl: " .. err
-    end
-
-    core.log.info("ttl key: ", key, " ret: ", ret, " err: ", err)
-    if ret < 0 then
-        local lock, err = resty_lock:new("plugin-limit-count")
-        if not lock then
-            return false, "failed to create lock: " .. err
-        end
-
-        local elapsed, err = lock:lock(key)
-        if not elapsed then
-            return false, "failed to acquire the lock: " .. err
-        end
-
-        ret = red:ttl(key)
-        if ret < 0 then
-            local ok, err = lock:unlock()
-            if not ok then
-                return false, "failed to unlock: " .. err
-            end
-
-            ret, err = red:set(key, limit -1, "EX", window)
-            if not ret then
-                return nil, err
-            end
+    local remaining,err = red:eval(script,1,key,limit,window)

Review comment:
       ok. thanks

##########
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##########
@@ -72,62 +71,28 @@ function _M.new(plugin_name, limit, window, conf)
     return setmetatable(self, mt)
 end
 
+local script = "if redis.call('ttl',KEYS[1]) < 0 then "

Review comment:
       I think ttl is better. ttl will set the expire time on the key which is permanent. Both are O(1) Time complexity.




-- 
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