code review

apisix/plugins/limit-conn.lua

@@ -20,7 +20,7 @@ local limit_conn = require("apisix.plugins.limit-conn.init")
 
 local plugin_name = "limit-conn"
 
-local counter_type_to_additional_properties = {
+local policy_to_additional_properties = {
     redis = {
         properties = {
             redis_host = {
@@ -103,7 +103,7 @@ local schema = {
             enum = {"var", "var_combination"},
             default = "var",
         },
-        counter_type = {
+        policy = {
             type = "string",
             enum = {"redis", "redis-cluster", "shared-dict"},
             default = "shared-dict",
@@ -119,30 +119,30 @@ local schema = {
     required = {"conn", "burst", "default_conn_delay", "key"},
     ["if"] = {
         properties = {
-            counter_type = {
+            policy = {
                 enum = {"redis"},
             },
         },
     },
-    ["then"] = counter_type_to_additional_properties.redis,
+    ["then"] = policy_to_additional_properties.redis,
     ["else"] = {
         ["if"] = {
             properties = {
-                counter_type = {
+                policy = {
                     enum = {"shared-dict"},
                 },
             },
         },
-        ["then"] = counter_type_to_additional_properties["shared-dict"],
+        ["then"] = policy_to_additional_properties["shared-dict"],
         ["else"] = {
             ["if"] = {
                 properties = {
-                    counter_type = {
+                    policy = {
                         enum = {"redis-cluster"},
                     },
                 },
             },
-            ["then"] = counter_type_to_additional_properties["redis-cluster"],
+            ["then"] = policy_to_additional_properties["redis-cluster"],
         }
     }
 }

apisix/plugins/limit-conn/init.lua

@@ -41,25 +41,25 @@ local _M = {}
 
 
 local function create_limit_obj(conf)
-    if conf.counter_type == "shared-dict" then
+    if conf.policy == "shared-dict" then
         core.log.info("create new limit-conn plugin instance")
         return limit_conn_new(shdict_name, conf.conn, conf.burst,
                               conf.default_conn_delay)
-    elseif conf.counter_type == "redis" then
+    elseif conf.policy == "redis" then
 
         core.log.info("create new limit-conn redis plugin instance")
 
         return redis_single_new("plugin-limit-conn", conf, conf.conn, conf.burst,
                                 conf.default_conn_delay)
 
-    elseif conf.counter_type == "redis-cluster" then
+    elseif conf.policy == "redis-cluster" then
 
         core.log.info("create new limit-conn redis-cluster plugin instance")
 
         return redis_cluster_new("plugin-limit-conn", conf, conf.conn, conf.burst,
                                  conf.default_conn_delay)
     else
-        return nil, "counter_type enum not match"
+        return nil, "policy enum not match"
     end
 end
 

apisix/plugins/limit-conn/limit-conn-redis-cluster.lua

@@ -1,10 +1,9 @@
-local rediscluster      = require("resty.rediscluster")
+local redis_cluster     = require("apisix.utils.rediscluster")
 local core              = require("apisix.core")
 local assert            = assert
 local setmetatable      = setmetatable
 local math              = require "math"
 local floor             = math.floor
-local ipairs            = ipairs
 local ngx_timer_at      = ngx.timer.at
 
 local _M = {version = 0.1}
@@ -15,59 +14,28 @@ local mt = {
 }
 
 
-local function new_redis_cluster(conf)
-    local config = {
-        name = conf.redis_cluster_name,
-        serv_list = {},
-        read_timeout = conf.redis_timeout,
-        auth = conf.redis_password,
-        dict_name = "plugin-limit-conn-redis-cluster-slot-lock",
-        connect_opts = {
-            ssl = conf.redis_cluster_ssl,
-            ssl_verify = conf.redis_cluster_ssl_verify,
-        }
-    }
-
-    for i, conf_item in ipairs(conf.redis_cluster_nodes) do
-        local host, port, err = core.utils.parse_addr(conf_item)
-        if err then
-            return nil, "failed to parse address: " .. conf_item
-                        .. " err: " .. err
-        end
-
-        config.serv_list[i] = {ip = host, port = port}
-    end
+function _M.new(plugin_name, conf, max, burst, default_conn_delay)
 
-    local red_cli, err = rediscluster:new(config)
+    local red_cli, err = redis_cluster.new(conf)
     if not red_cli then
-        return nil, "failed to new redis cluster: " .. err
+        return nil, err
     end
-
-    return red_cli
-end
-
-
-function _M.new(plugin_name, conf, max, burst, default_conn_delay)
     local self = {
         conf = conf,
         plugin_name = plugin_name,
         burst = burst,
         max = max + 0,    -- just to ensure the param is good
         unit_delay = default_conn_delay,
+        red_cli = red_cli,
     }
     return setmetatable(self, mt)
 end
 
 
 function _M.incoming(self, key, commit)
     local max = self.max
-
-    -- init redis
+    local red = self.red_cli
     local conf = self.conf
-    local red, err = new_redis_cluster(conf)
-    if not red then
-        return red, err
-    end
 
     self.committed = false
 
@@ -116,11 +84,7 @@ end
 local function leaving_thread(premature, self, key, req_latency)
 
     -- init redis
-    local conf = self.conf
-    local red, err = new_redis_cluster(conf)
-    if not red then
-        return red, err
-    end
+    local red = self.red_cli
 
     local prefix = conf.redis_prefix
     local hash_key = prefix .. ":connection_hash"

apisix/plugins/limit-conn/limit-conn-redis-single.lua

@@ -1,9 +1,9 @@
-local redis_new     = require("resty.redis").new
+local redis     = require("apisix.utils.redis")
 local core          = require("apisix.core")
 local assert        = assert
 local math          = require "math"
 local floor         = math.floor
-local ngx_timer_at   = ngx.timer.at
+local ngx_timer_at  = ngx.timer.at
 
 local setmetatable  = setmetatable
 
@@ -15,53 +15,6 @@ local mt = {
     __index = _M
 }
 
-
-local function redis_cli(conf)
-    local red = redis_new()
-    local timeout = conf.redis_timeout or 1000    -- default 1sec
-
-    red:set_timeouts(timeout, timeout, timeout)
-
-    local sock_opts = {
-        ssl = conf.redis_ssl,
-        ssl_verify = conf.redis_ssl_verify
-    }
-
-    local ok, err = red:connect(conf.redis_host, conf.redis_port or 6379, sock_opts)
-    if not ok then
-        core.log.error(" redis connect error, error: ", err)
-        return false, err
-    end
-
-    local count
-    count, err = red:get_reused_times()
-    if 0 == count then
-        if conf.redis_password and conf.redis_password ~= '' then
-            local ok, err
-            if conf.redis_username then
-                ok, err = red:auth(conf.redis_username, conf.redis_password)
-            else
-                ok, err = red:auth(conf.redis_password)
-            end
-            if not ok then
-                return nil, err
-            end
-        end
-
-        -- select db
-        if conf.redis_database ~= 0 then
-            local ok, err = red:select(conf.redis_database)
-            if not ok then
-                return false, "failed to change redis db, err: " .. err
-            end
-        end
-    elseif err then
-        -- core.log.info(" err: ", err)
-        return nil, err
-    end
-    return red, nil
-end
-
 function _M.new(plugin_name, conf, max, burst, default_conn_delay)
 
     local self = {
@@ -80,7 +33,7 @@ function _M.incoming(self, key, commit)
 
     -- init redis
     local conf = self.conf
-    local red, err = redis_cli(conf)
+    local red, err = redis.new(conf)
     if not red then
         return red, err
     end
@@ -133,7 +86,7 @@ local function leaving_thread(premature, self, key, req_latency)
 
     -- init redis
     local conf = self.conf
-    local red, err = redis_cli(conf)
+    local red, err = redis.new(conf)
     if not red then
         return red, err
     end

apisix/utils/redis.lua

@@ -0,0 +1,59 @@
+local redis_new     = require("resty.redis").new
+local core          = require("apisix.core")
+
+
+local _M = {version = 0.1}
+
+local function redis_cli(conf)
+    local red = redis_new()
+    local timeout = conf.redis_timeout or 1000    -- default 1sec
+
+    red:set_timeouts(timeout, timeout, timeout)
+
+    local sock_opts = {
+        ssl = conf.redis_ssl,
+        ssl_verify = conf.redis_ssl_verify
+    }
+
+    local ok, err = red:connect(conf.redis_host, conf.redis_port or 6379, sock_opts)
+    if not ok then
+        core.log.error(" redis connect error, error: ", err)
+        return false, err
+    end
+
+    local count
+    count, err = red:get_reused_times()
+    if 0 == count then
+        if conf.redis_password and conf.redis_password ~= '' then
+            local ok, err
+            if conf.redis_username then
+                ok, err = red:auth(conf.redis_username, conf.redis_password)
+            else
+                ok, err = red:auth(conf.redis_password)
+            end
+            if not ok then
+                return nil, err
+            end
+        end
+
+        -- select db
+        if conf.redis_database ~= 0 then
+            local ok, err = red:select(conf.redis_database)
+            if not ok then
+                return false, "failed to change redis db, err: " .. err
+            end
+        end
+    elseif err then
+        -- core.log.info(" err: ", err)
+        return nil, err
+    end
+    return red, nil
+end
+
+
+
+function _M.new(conf)
+    return redis_cli(conf)
+end
+
+return _M

apisix/utils/rediscluster.lua

@@ -0,0 +1,44 @@
+local rediscluster      = require("resty.rediscluster")
+local core              = require("apisix.core")
+local ipairs            = ipairs
+
+local _M = {version = 0.1}
+
+local function new_redis_cluster(conf)
+    local config = {
+        name = conf.redis_cluster_name,
+        serv_list = {},
+        read_timeout = conf.redis_timeout,
+        auth = conf.redis_password,
+        dict_name = "plugin-limit-conn-redis-cluster-slot-lock",
+        connect_opts = {
+            ssl = conf.redis_cluster_ssl,
+            ssl_verify = conf.redis_cluster_ssl_verify,
+        }
+    }
+
+    for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+        local host, port, err = core.utils.parse_addr(conf_item)
+        if err then
+            return nil, "failed to parse address: " .. conf_item
+                        .. " err: " .. err
+        end
+
+        config.serv_list[i] = {ip = host, port = port}
+    end
+
+    local red_cli, err = rediscluster:new(config)
+    if not red_cli then
+        return nil, "failed to new redis cluster: " .. err
+    end
+
+    return red_cli
+end
+
+
+function _M.new(conf)
+     return new_redis_cluster(conf)
+end
+
+
+return _M