[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-05 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r499664897



##
File path: rockspec/apisix-master-0.rockspec
##
@@ -54,6 +54,7 @@ dependencies = {
 "skywalking-nginx-lua-plugin = 1.0-0",
 "base64 = 1.5-2",
 "dkjson = 2.5-2",
+"lua-resty-redis-cluster = 1.1-0",

Review comment:
   https://luarocks.org/modules/steve0511/resty-redis-cluster
   
   we can use this library now. @liuhengloveyou 





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-08 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501548890



##
File path: apisix/plugins/limit-count.lua
##
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
 end
 
 if conf.policy == "redis-cluster" then
--- TODO
+if not conf.redis_serv_list then
+return false, "missing valid redis option serv_list"

Review comment:
   that is a wrong way, we can use JSONSchema to check this





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-08 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501548890



##
File path: apisix/plugins/limit-count.lua
##
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
 end
 
 if conf.policy == "redis-cluster" then
--- TODO
+if not conf.redis_serv_list then
+return false, "missing valid redis option serv_list"

Review comment:
   we can use a better way, use JSONSchema to check this





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-08 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501552477



##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+dict_name = "redis_cluster_slot_locks", --shared dictionary name for 
locks
+name = "apisix-rediscluster",   --rediscluster name
+keepalive_timeout = 6,  --redis connection pool idle 
timeout
+keepalive_cons = 1000,  --redis connection pool size
+connect_timeout = 1000, --timeout while connecting
+max_redirection = 5,--maximum retry attempts for 
redirection
+max_connection_attempts = 1,--maximum retry attempts for 
connection
+read_timeout = conf.redis_timeout or 1000,
+enable_slave_read = true,
+serv_list = {},
+}
+
+for key, value in ipairs(conf.redis_serv_list) do
+if value['redis_host'] and value['redis_port'] then
+config.serv_list[key] = {ip = value['redis_host'], port = 
value['redis_port']}
+end
+end
+
+if conf.redis_password then
+config.auth = conf.redis_password --set password while setting auth
+end
+
+local redis_cluster = require "resty.rediscluster"
+local red_c = redis_cluster:new(config)
+
+return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+assert(limit > 0 and window > 0)
+
+_M.red_c = new_redis_cluster(conf)
+
+local self = {limit = limit, window = window, conf = conf,
+  plugin_name = plugin_name}
+return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+local conf = self.conf
+local red = _M.red_c
+core.log.info("ttl key: ", key, " timeout: ", conf.redis_timeout or 1000)

Review comment:
   use JSONSchema to set the default value

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+dict_name = "redis_cluster_slot_locks", --shared dictionary name for 
locks
+name = "apisix-rediscluster",   --rediscluster name
+keepalive_timeout = 6,  --redis connection pool idle 
timeout
+keepalive_cons = 1000,  --redis connection pool size
+connect_timeout = 1000, --timeout while connecting
+max_redirection = 5,--maximum retry attempts for 
redirection
+max_connection_attempts = 1,--maximum retry attempts for 
connection
+read_timeout = conf.re

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-09 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502279935



##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs  = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+dict_name = "redis_cluster_slot_locks", --shared dictionary name for 
locks
+name = "apisix-rediscluster",   --rediscluster name
+keepalive_timeout = 6,  --redis connection pool idle 
timeout
+keepalive_cons = 1000,  --redis connection pool size
+connect_timeout = 1000, --timeout while connecting
+max_redirection = 5,--maximum retry attempts for 
redirection
+max_connection_attempts = 1,--maximum retry attempts for 
connection
+read_timeout = conf.redis_timeout or 1000,
+enable_slave_read = true,
+serv_list = {},
+}
+
+for key, value in ipairs(conf.redis_serv_list) do
+if value['redis_host'] and value['redis_port'] then
+config.serv_list[key] = {ip = value['redis_host'], port = 
value['redis_port']}
+end
+end
+
+if conf.redis_password then
+config.auth = conf.redis_password --set password while setting auth
+end
+
+local redis_cluster = require "resty.rediscluster"
+local red_c = redis_cluster:new(config)

Review comment:
   I think it may fail, need to capture the error message

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs  = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+dict_name = "redis_cluster_slot_locks", --shared dictionary name for 
locks
+name = "apisix-rediscluster",   --rediscluster name
+keepalive_timeout = 6,  --redis connection pool idle 
timeout

Review comment:
   can we use the default value here?

##
File path: doc/plugins/limit-count.md
##
@@ -109,6 +110,41 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 
'X-API-KEY: edd1c9f034335
 }'
 ```
 
+If using redis-cluster:
+
+```shell
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: 
edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+"uri": "/index.html",
+"plugins": {
+"limit-count": {
+"count": 2,
+"time_window": 60,
+"rejected_code": 503,
+"key": "remote_addr",
+"policy": "redis-cluster",
+"redis_serv_list": [
+   {

Review comment:
   how about this style?

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-09 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r501548890



##
File path: apisix/plugins/limit-count.lua
##
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
 end
 
 if conf.policy == "redis-cluster" then
--- TODO
+if not conf.redis_serv_list then
+return false, "missing valid redis option serv_list"

Review comment:
   that is a wrong way, we can use JSONSchema to check this

##
File path: apisix/plugins/limit-count.lua
##
@@ -132,7 +132,9 @@ function _M.check_schema(conf)
 end
 
 if conf.policy == "redis-cluster" then
--- TODO
+if not conf.redis_serv_list then
+return false, "missing valid redis option serv_list"

Review comment:
   we can use a better way, use JSONSchema to check this

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+dict_name = "redis_cluster_slot_locks", --shared dictionary name for 
locks
+name = "apisix-rediscluster",   --rediscluster name
+keepalive_timeout = 6,  --redis connection pool idle 
timeout
+keepalive_cons = 1000,  --redis connection pool size
+connect_timeout = 1000, --timeout while connecting
+max_redirection = 5,--maximum retry attempts for 
redirection
+max_connection_attempts = 1,--maximum retry attempts for 
connection
+read_timeout = conf.redis_timeout or 1000,
+enable_slave_read = true,
+serv_list = {},
+}
+
+for key, value in ipairs(conf.redis_serv_list) do
+if value['redis_host'] and value['redis_port'] then
+config.serv_list[key] = {ip = value['redis_host'], port = 
value['redis_port']}
+end
+end
+
+if conf.redis_password then
+config.auth = conf.redis_password --set password while setting auth
+end
+
+local redis_cluster = require "resty.rediscluster"
+local red_c = redis_cluster:new(config)
+
+return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+assert(limit > 0 and window > 0)
+
+_M.red_c = new_redis_cluster(conf)
+
+local self = {limit = limit, window = window, conf = conf,
+  plugin_name = plugin_name}
+return setmetatable(self, mt)
+
+end
+
+
+function _M.incoming(self, key)
+local conf = self.conf
+local red = _M.red_c
+core.log.info("ttl key: ", key, " timeout: ", conf.redis_timeout or 1000)

Review comment:
   use JSONSchema to set the default value

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,138 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+
+
+local _M = {version = 0.3}
+
+
+local m

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-09 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502721349



##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs  = ipairs
+
+
+local _M = {version = 0.3}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+dict_name = "redis_cluster_slot_locks", --shared dictionary name for 
locks
+name = "apisix-rediscluster",   --rediscluster name
+keepalive_timeout = 6,  --redis connection pool idle 
timeout

Review comment:
   ping  @liuhengloveyou 





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-09 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502743834



##
File path: doc/zh-cn/plugins/limit-count.md
##
@@ -26,17 +26,18 @@
 
 ## 参数
 
-| 名称   | 类型| 必选项   | 默认值  | 有效值
   | 描述 

   |
-| -- | --- |  | --- | 
 | 
---
 |
-| count  | integer | 必须 | | [0,...]
  | 指定时间窗口内的请求数量阈值  

  |
-| time_window| integer | 必须 | | [0,...]
  | 时间窗口的大小（以秒为单位），超过这个时间就会重置   

   |
-| key| string  | 必须 | | ["remote_addr", 
"server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据   

 |
-| rejected_code  | integer | 可选 | 503 | [200,600]  
  | 当请求超过阈值被拒绝时，返回的 HTTP 状态码

  |
-| policy | string  | 可选 | "local" | ["local", "redis"] 
  | 
用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 
服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速) |
-| redis_host | string  | `redis` 必须 | |
  | 当使用 `redis` 限速策略时，该属性是 Redis 
服务节点的地址。
  |
-| redis_port | integer | 可选 | 6379| [1,...]
  | 当使用 `redis` 限速策略时，该属性是 Redis 
服务节点的端口 
   |
-| redis_password | string  | 可选 | |
  | 当使用 `redis` 限速策略时，该属性是 Redis 
服务节点的密码。
  |
-| redis_timeout  | integer | 可选 | 1000| [1,...]
  | 当使用 `redis` 限速策略时，该属性是 Redis 
服务节点以毫秒为单位的超时时间 
   |
+| 名称| 类型 | 必选项   | 默认值  | 有效值  
 | 描述   
  |
+| --- |  |  | --- | 
 | 
 |
+| count   | integer  | 必须 | | [0,...]  
| 指定时间窗口内的请求数量阈值
 |
+| time_window | integer  | 必须 | | [0,...]  
| 时间窗口的大小（以秒为单位），超过这个时间就会重置   |
+| key | string   | 必须 | | ["remote_addr", 
"server_addr", "http_x_real_ip", "http_x_forwarded_for"] | 用来做请求计数的依据   
  |
+| rejected_code   | integer  | 可选 | 503 | [200,600]
| 当请求超过阈值被拒绝时，返回的 HTTP 状态码  
 |
+| policy  | string   | 可选 | "local" | ["local", "redis", 
"redis-cluster"]  | 
用于检索和增加限制的速率限制策略。可选的值有：`local`(计数器被以内存方式保存在节点本地，默认选项) 和 `redis`(计数器保存在 Redis 
服务节点上，从而可以跨节点共享结果，通常用它来完成全局限速)；以及`redis-cluster`，跟redis功能一样，只是使用redis集群方式。 |

Review comment:
   ping @liuhengloveyou 

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,139 @@
+--
+-- Licensed to the Apach

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-10 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502777237



##
File path: apisix/plugins/limit-count.lua
##
@@ -119,6 +148,11 @@ local function create_limit_obj(conf)
conf.count, conf.time_window, conf)
 end
 
+if conf.policy == "redis-cluster" then
+return limit_redis_cluster_new("plugin-" .. plugin_name,
+   conf.count, conf.time_window, conf)

Review comment:
   bad indentation

##
File path: apisix/plugins/limit-count.lua
##
@@ -70,11 +74,36 @@ local schema = {
 type = "string", minLength = 0,
 },
 redis_timeout = {
-type = "integer", minimum = 1,
-default = 1000,
+type = "integer", minimum = 1, default = 1000,
 },
 },
 required = {"redis_host"},
+},
+{
+properties = {
+policy = {
+enum = {"redis-cluster"},
+},
+redis_serv_list = {
+type = "array",
+minItems = 2,
+items = {
+type = "object",
+properties = {
+redis_host = {type = "string", minLength = 
2},
+redis_port = {type = "integer", minimum = 
1},

Review comment:
   `max` ?

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local require = require
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+name = "apisix-rediscluster",   --rediscluster name
+enable_slave_read = true,
+keepalive_timeout = 6,  --redis connection pool idle 
timeout
+keepalive_cons = 1000,  --redis connection pool size
+connect_timeout = 1000, --timeout while connecting
+send_timeout = 1000,--timeout while sending
+max_redirection = 5,--maximum retry attempts for 
redirection
+max_connection_attempts = 1,--maximum retry attempts for 
connection
+serv_list = {},
+read_timeout = conf.redis_timeout,
+auth = conf.redis_password --set password while setting auth
+}
+
+for key, value in ipairs(conf.redis_serv_list) do
+if value['redis_host'] and value['redis_port'] then
+config.serv_list[key] = {ip = value['redis_host'], port = 
value['redis_port']}
+end
+end
+
+
+local redis_cluster = require "resty.rediscluster"
+local red_c = redis_cluster:new(config)
+if not red_c then
+error("connect to redis cluster fails")
+end
+
+return red_c
+end
+
+
+function _M.new(plugin_name, limit, window, conf)
+assert(limit > 0 and window > 0)
+
+_M.red_c = new_redis_cluster(conf)

Review comment:
   that is wrong!! a bug, we need to fix it

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,141 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the Licens

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-10 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502787623



##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}

Review comment:
   this version is useless, we can remove it

##
File path: apisix/plugins/limit-count.lua
##
@@ -119,6 +148,11 @@ local function create_limit_obj(conf)
conf.count, conf.time_window, conf)
 end
 
+if conf.policy == "redis-cluster" then
+return limit_redis_cluster_new("plugin-" .. plugin_name,conf.count,

Review comment:
   style: need a space between different argument items

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,127 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+
+local _M = {version = 0.1}
+
+
+local mt = {
+__index = _M
+}
+
+-- https://github.com/steve0511/resty-redis-cluster
+local function new_redis_cluster(conf)
+local config = {
+name = "apisix-rediscluster",
+serv_list = {},
+read_timeout = conf.redis_timeout,
+auth = conf.redis_password
+}
+
+for i, c in ipairs(conf.redis_serv_list) do
+config.serv_list[i] = {ip = c.host, port = c.port}
+end
+
+local red_cli = rediscluster:new(config)
+if not red_cli then
+error("connect to redis cluster fails")

Review comment:
   Please confirm if we can capture more error messages

##
File path: doc/zh-cn/plugins/limit-count.md
##
@@ -26,17 +26,18 @@
 
 ## 参数
 
-| 名称   | 类型| 必选项   | 默认值  | 有效值
   | 描述 

   |
-| -- | --- |  | --- | 
 | 
---
 |
-| count  | integer | 必须 | | [0,...]
  | 指定时间窗口内的请求数量阈值  

  |
-| time_window| integer | 必须 | | [0,...]
  | 时间窗口的大小（以秒为单位），超过这个时间就会重置   

   |
-| key

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-10 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502794290



##
File path: rockspec/apisix-master-0.rockspec
##
@@ -54,6 +54,7 @@ dependencies = {
 "skywalking-nginx-lua-plugin = 1.0-0",
 "base64 = 1.5-2",
 "dkjson = 2.5-2",
+"resty-redis-cluster = 1.02-4",

Review comment:
   it is the right version





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-10 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r502795882



##
File path: rockspec/apisix-master-0.rockspec
##
@@ -54,6 +54,7 @@ dependencies = {
 "skywalking-nginx-lua-plugin = 1.0-0",
 "base64 = 1.5-2",
 "dkjson = 2.5-2",
+"resty-redis-cluster = 1.02-4",

Review comment:
   https://luarocks.org/modules/steve0511/resty-redis-cluster





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-12 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503373211



##
File path: bin/apisix
##
@@ -190,6 +190,7 @@ http {
 lua_shared_dict balancer_ewma10m;
 lua_shared_dict balancer_ewma_locks  10m;
 lua_shared_dict balancer_ewma_last_touched_at 10m;
+lua_shared_dict redis_cluster_slot_locks 100k;

Review comment:
   that is not a good plugin name, we should add a prefix about the plugin 
name





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-12 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503374436



##
File path: t/plugin/limit-count-redis-cluster.t
##
@@ -0,0 +1,228 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+BEGIN {
+if ($ENV{TEST_NGINX_CHECK_LEAK}) {
+$SkipReason = "unavailable for the hup tests";
+
+} else {
+$ENV{TEST_NGINX_USE_HUP} = 1;
+undef $ENV{TEST_NGINX_USE_STAP};
+}
+}
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_shuffle();
+no_root_location();
+run_tests;

Review comment:
   one blank line before this line





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-12 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503374195



##
File path: t/plugin/limit-count-redis-cluster.t
##
@@ -0,0 +1,228 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+BEGIN {

Review comment:
   they were useless I think, we should remove them





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-12 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503707079



##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+__index = _M
+}
+
+
+local function new_redis_cluster(conf)
+local config = {
+name = "apisix-redis-cluster",
+serv_list = {},
+read_timeout = conf.redis_timeout,
+auth = conf.redis_password
+}
+
+for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+local host, port, err = core.utils.parse_addr(conf_item)
+if err then
+error("limit-count-redis: redis_cluster_nodes configure err " .. 
conf_item, err)

Review comment:
   I this is wrong

##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+__index = _M
+}
+
+
+local function new_redis_cluster(conf)
+local config = {
+name = "apisix-redis-cluster",
+serv_list = {},
+read_timeout = conf.redis_timeout,
+auth = conf.redis_password
+}
+
+for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+local host, port, err = core.utils.parse_addr(conf_item)
+if err then
+error("limit-count-redis: redis_cluster_nodes configure err " .. 
conf_item, err)
+end
+
+config.serv_list[i] = {ip = host, port = port}
+end
+
+local red_cli, err = rediscluster:new(config)
+if not red_cli then
+error("limit-count-redis: connect to redis cluster failed: ", err)

Review comment:
   ditto





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:
us...@infra.apache.org

[GitHub] [apisix] membphis commented on a change in pull request #2340: feature: limit-count use redis cluster

2020-10-13 Thread GitBox 


membphis commented on a change in pull request #2340:
URL: https://github.com/apache/apisix/pull/2340#discussion_r503707079



##
File path: apisix/plugins/limit-count/limit-count-redis-cluster.lua
##
@@ -0,0 +1,130 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements.  See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License.  You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local rediscluster = require("resty.rediscluster")
+local core = require("apisix.core")
+local resty_lock = require("resty.lock")
+local assert = assert
+local error = error
+local setmetatable = setmetatable
+local tostring = tostring
+local ipairs = ipairs
+
+local _M = {}
+
+local mt = {
+__index = _M
+}
+
+
+local function new_redis_cluster(conf)
+local config = {
+name = "apisix-redis-cluster",
+serv_list = {},
+read_timeout = conf.redis_timeout,
+auth = conf.redis_password
+}
+
+for i, conf_item in ipairs(conf.redis_cluster_nodes) do
+local host, port, err = core.utils.parse_addr(conf_item)
+if err then
+error("limit-count-redis: redis_cluster_nodes configure err " .. 
conf_item, err)

Review comment:
   I think this is wrong





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:
us...@infra.apache.org