You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "shreemaan-abhishek (via GitHub)" <gi...@apache.org> on 2023/04/12 05:23:40 UTC
[GitHub] [apisix] shreemaan-abhishek opened a new pull request, #9291: feat: make limit-count/init.lua hookable
shreemaan-abhishek opened a new pull request, #9291:
URL: https://github.com/apache/apisix/pull/9291
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang merged pull request #9291: feat: support consuming specified cost in the limit count function
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang merged PR #9291:
URL: https://github.com/apache/apisix/pull/9291
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1168078005
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -112,5 +113,5 @@ function _M.incoming(self, key)
return 0, remaining, ttl
end
-
+_M.script = script
Review Comment:
I mean don't export the script, keep it as it is
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek closed pull request #9291: feat: make limit-count/init.lua hookable
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek closed pull request #9291: feat: make limit-count/init.lua hookable
URL: https://github.com/apache/apisix/pull/9291
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1167047162
##########
t/plugin/limit-count4.t:
##########
@@ -171,3 +171,57 @@ passed
}
--- response_body
["1","0","0"]
+
+
+
+=== TEST 5: modified limit-count.incoming, cost == 2
+--- config
+ location = /t {
+ content_by_lua_block {
+ local limit_count_local = require "apisix.plugins.limit-count.limit-count-local"
+ local lim = limit_count_local.new("plugin-limit-count", 10, 60)
+ local uri = ngx.var.uri
+ for i = 1, 7 do
+ local delay, err = lim.limit_count:handle_incoming(uri, 2, true)
Review Comment:
Addressed the issue, please see if the tests are up to the mark.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1165043440
##########
t/plugin/limit-count4.t:
##########
@@ -171,3 +171,57 @@ passed
}
--- response_body
["1","0","0"]
+
+
+
+=== TEST 5: modified limit-count.incoming, cost == 2
+--- config
+ location = /t {
+ content_by_lua_block {
+ local limit_count_local = require "apisix.plugins.limit-count.limit-count-local"
+ local lim = limit_count_local.new("plugin-limit-count", 10, 60)
+ local uri = ngx.var.uri
+ for i = 1, 7 do
+ local delay, err = lim.limit_count:handle_incoming(uri, 2, true)
Review Comment:
I know, we hook it in a very hacky way. But essentially it doesn't belong to apisix, if you only test it, some logic in the incoming function will be missed
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9291: feat: support consuming specified cost in the limit count function
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1173233472
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -20,7 +20,6 @@ local tab_insert = table.insert
local ipairs = ipairs
local pairs = pairs
-local plugin_name = "limit-count"
Review Comment:
The code here is a public capability of apisix, not a plugin. The plugin name should be defined by the plugin using the capability
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1164953338
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -112,5 +113,5 @@ function _M.incoming(self, key)
return 0, remaining, ttl
end
-
+_M.script = script
Review Comment:
The redis script in `limit-count-redis.lua` and `limit-count-redis-cluster.lua` are the same so I exported it to avoid duplication.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1168084498
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -112,5 +113,5 @@ function _M.incoming(self, key)
return 0, remaining, ttl
end
-
+_M.script = script
Review Comment:
Ah sorry let me fix it.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] AlinsRan commented on a diff in pull request #9291: feat: support consuming specified cost in the limit count function
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1173228255
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -20,7 +20,6 @@ local tab_insert = table.insert
local ipairs = ipairs
local pairs = pairs
-local plugin_name = "limit-count"
Review Comment:
The plugin name will not change.
It seems unnecessary to repeatedly pass between multiple functions and concatenate strings.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1165041405
##########
t/plugin/limit-count4.t:
##########
@@ -171,3 +171,57 @@ passed
}
--- response_body
["1","0","0"]
+
+
+
+=== TEST 5: modified limit-count.incoming, cost == 2
+--- config
+ location = /t {
+ content_by_lua_block {
+ local limit_count_local = require "apisix.plugins.limit-count.limit-count-local"
+ local lim = limit_count_local.new("plugin-limit-count", 10, 60)
+ local uri = ngx.var.uri
+ for i = 1, 7 do
+ local delay, err = lim.limit_count:handle_incoming(uri, 2, true)
Review Comment:
In fact, the `handle_incoming` is a part of APISIX. I extended the existing function from lua-resty-limit-traffic.
https://github.com/apache/apisix/blob/f294304b61f104db6ef3cff9cc26e9a36d125824/apisix/plugins/limit-count/limit-count-local.lua#L19-L43
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1164950917
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -112,5 +113,5 @@ function _M.incoming(self, key)
return 0, remaining, ttl
end
-
+_M.script = script
Review Comment:
Why export 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1170254672
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -14,7 +14,34 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local limit_local_new = require("resty.limit.count").new
+local limit_count = require("resty.limit.count")
+
+limit_count.handle_incoming = function (self, key, cost, commit)
+ if cost < 1 then
+ return nil, "cost must be at least 1"
+ end
+ local dict = self.dict
+ local limit = self.limit
+ local window = self.window
+
Review Comment:
Nevertheless, we HAVE to maintain a separate codebase. And in fact, we are just adding a new function which is essentially a modified version of the existing `incoming` function.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #9291: feat: make limit-count/init.lua hookable
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on PR #9291:
URL: https://github.com/apache/apisix/pull/9291#issuecomment-1504741002
Can you add some test cases？
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1170185319
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -14,7 +14,34 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local limit_local_new = require("resty.limit.count").new
+local limit_count = require("resty.limit.count")
+
+limit_count.handle_incoming = function (self, key, cost, commit)
+ if cost < 1 then
+ return nil, "cost must be at least 1"
+ end
+ local dict = self.dict
+ local limit = self.limit
+ local window = self.window
+
Review Comment:
I think we'd better not hack the underlying functions and suggest directly fork the corresponding repositories to make changes
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1168091741
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -112,5 +113,5 @@ function _M.incoming(self, key)
return 0, remaining, ttl
end
-
+_M.script = script
Review Comment:
FYI, we can't test the script without exporting it, so I removed the tests as well.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1165036843
##########
t/plugin/limit-count4.t:
##########
@@ -171,3 +171,57 @@ passed
}
--- response_body
["1","0","0"]
+
+
+
+=== TEST 5: modified limit-count.incoming, cost == 2
+--- config
+ location = /t {
+ content_by_lua_block {
+ local limit_count_local = require "apisix.plugins.limit-count.limit-count-local"
+ local lim = limit_count_local.new("plugin-limit-count", 10, 60)
+ local uri = ngx.var.uri
+ for i = 1, 7 do
+ local delay, err = lim.limit_count:handle_incoming(uri, 2, true)
Review Comment:
Please add test cases to test the incoming function separately, limit_count.handle_incoming is not a module function of apisix.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9291: chore: make limit-count/init.lua hookable
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9291:
URL: https://github.com/apache/apisix/pull/9291#discussion_r1164956488
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -112,5 +113,5 @@ function _M.incoming(self, key)
return 0, remaining, ttl
end
-
+_M.script = script
Review Comment:
If you want to export this, it's better to put it in init.lua, don't refer to each other between parallel files. But it doesn't seem appropriate to put it in init.lua, so let's keep it as it is
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org