shreemaan-abhishek commented on code in PR #10866: URL: https://github.com/apache/apisix/pull/10866#discussion_r1471481910
########## apisix/plugins/limit-conn/limit-conn-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 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 ngx_timer_at = ngx.timer.at + +local _M = {version = 0.1} + + +local mt = { + __index = _M +} + + +function _M.new(plugin_name, conf, max, burst, default_conn_delay) + + local red_cli, err = redis_cluster.new(conf) + if not red_cli then + return nil, err + end + 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 + local red = self.red_cli + local conf = self.conf + + self.committed = false + + local prefix = conf.redis_prefix + local hash_key = prefix .. ":connection_hash" + + local conn, err + if commit then + conn, err = red:hincrby(hash_key, key, 1) + if not conn then + return nil, err + end + + if conn > max + self.burst then + conn, err = red:hincrby(hash_key, key, -1) + if not conn then + return nil, err + end + return nil, "rejected" + end + self.committed = true + + else + local conn_from_red, err = red:hget(hash_key, key) + if err then + return nil, err + end + conn = (conn_from_red or 0) + 1 + end + + if conn > max then + -- make the excessive connections wait + return self.unit_delay * floor((conn - 1) / max), conn + end + + -- we return a 0 delay by default + return 0, conn +end + + +function _M.is_committed(self) + return self.committed +end + + +local function leaving_thread(premature, self, key, req_latency) + + -- init redis Review Comment: remove this ########## apisix/plugins/limit-conn.lua: ########## @@ -14,23 +14,41 @@ -- See the License for the specific language governing permissions and -- limitations under the License. -- -local core = require("apisix.core") -local limit_conn = require("apisix.plugins.limit-conn.init") +local core = require("apisix.core") +local limit_conn = require("apisix.plugins.limit-conn.init") +local redis_schema = require("apisix.utils.redis-schema") +local policy_to_additional_properties = redis_schema.schema +local plugin_name = "limit-conn" -local plugin_name = "limit-conn" +policy_to_additional_properties['local'] = { + properties = { + }, +} + local schema = { type = "object", properties = { - conn = {type = "integer", exclusiveMinimum = 0}, + conn = {type = "integer", exclusiveMinimum = 0}, -- limit.conn max burst = {type = "integer", minimum = 0}, default_conn_delay = {type = "number", exclusiveMinimum = 0}, only_use_default_delay = {type = "boolean", default = false}, - key = {type = "string"}, - key_type = {type = "string", + key = { + type = "string" + }, + key_type = { + type = "string", Review Comment: can we avoid making these changes? ########## apisix/plugins/limit-conn/limit-conn-redis-single.lua: ########## Review Comment: change the filename to `limit-conn-redis` ########## apisix/plugins/limit-conn/limit-conn-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 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 ngx_timer_at = ngx.timer.at + +local _M = {version = 0.1} + + +local mt = { + __index = _M +} + + +function _M.new(plugin_name, conf, max, burst, default_conn_delay) + + local red_cli, err = redis_cluster.new(conf) + if not red_cli then + return nil, err + end + 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 + local red = self.red_cli + local conf = self.conf + + self.committed = false + + local prefix = conf.redis_prefix + local hash_key = prefix .. ":connection_hash" + + local conn, err + if commit then + conn, err = red:hincrby(hash_key, key, 1) Review Comment: I couldn't find the declaration of this function `hincrby` can you help me find it? ########## apisix/utils/rediscluster.lua: ########## Review Comment: rediscluster.lua can be merged to redis.lua ########## apisix/plugins/limit-conn/limit-conn-redis-single.lua: ########## @@ -0,0 +1,143 @@ +-- +-- 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 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 setmetatable = setmetatable + + +local _M = {version = 0.1} + + +local mt = { + __index = _M +} + +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, + } + return setmetatable(self, mt) +end + + +function _M.incoming(self, key, commit) + local max = self.max + + -- init redis + local conf = self.conf + local red, err = redis.new(conf) Review Comment: why create a new redis client for each request? ########## apisix/plugins/limit-conn.lua: ########## @@ -14,23 +14,41 @@ -- See the License for the specific language governing permissions and -- limitations under the License. -- -local core = require("apisix.core") -local limit_conn = require("apisix.plugins.limit-conn.init") +local core = require("apisix.core") +local limit_conn = require("apisix.plugins.limit-conn.init") +local redis_schema = require("apisix.utils.redis-schema") +local policy_to_additional_properties = redis_schema.schema +local plugin_name = "limit-conn" -local plugin_name = "limit-conn" +policy_to_additional_properties['local'] = { Review Comment: why is this needed? ########## apisix/plugins/limit-conn/limit-conn-redis-single.lua: ########## @@ -0,0 +1,143 @@ +-- +-- 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 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 setmetatable = setmetatable + + +local _M = {version = 0.1} + + +local mt = { + __index = _M +} + +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, + } + return setmetatable(self, mt) +end + + +function _M.incoming(self, key, commit) + local max = self.max + + -- init redis + local conf = self.conf + local red, err = redis.new(conf) + if not red then + return red, err + end + + self.committed = false + + local prefix = conf.redis_prefix + local hash_key = prefix .. ":connection_hash" + + local conn, err + if commit then + conn, err = red:hincrby(hash_key, key, 1) + if not conn then + return nil, err + end + + if conn > max + self.burst then + conn, err = red:hincrby(hash_key, key, -1) + if not conn then + return nil, err + end + return nil, "rejected" + end + self.committed = true + + else + local conn_from_red, err = red:hget(hash_key, key) + if err then + return nil, err + end + conn = (conn_from_red or 0) + 1 + end + + if conn > max then + -- make the excessive connections wait + return self.unit_delay * floor((conn - 1) / max), conn + end + + -- we return a 0 delay by default + return 0, conn +end + + +function _M.is_committed(self) + return self.committed +end + + +local function leaving_thread(premature, self, key, req_latency) + + -- init redis + local conf = self.conf + local red, err = redis.new(conf) Review Comment: why create a new redis here again?? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
