You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/08/20 13:20:57 UTC
[GitHub] [apisix] spacewander opened a new pull request, #7750: fix(limit-count): different route with same conf should not share counter
spacewander opened a new pull request, #7750:
URL: https://github.com/apache/apisix/pull/7750
Signed-off-by: spacewander <sp...@gmail.com>
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### Checklist
- [x] 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
- [x] 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] tzssangglass commented on a diff in pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7750:
URL: https://github.com/apache/apisix/pull/7750#discussion_r950848127
##########
t/plugin/limit-count3.t:
##########
@@ -224,3 +224,64 @@ passed
}
--- response_body
[200,200]
+
+
+
+=== TEST 7: set another route with the same conf
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello1",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60
+ }
Review Comment:
but the `time_window = 60` in TEST 7 is different form the `time_window = 61` TEST 5?
It seems that TEST 5&7 should test the same conf and not affect each other.
--
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] spacewander commented on a diff in pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7750:
URL: https://github.com/apache/apisix/pull/7750#discussion_r950943944
##########
t/plugin/limit-count3.t:
##########
@@ -224,3 +224,64 @@ passed
}
--- response_body
[200,200]
+
+
+
+=== TEST 7: set another route with the same conf
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello1",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60
+ }
Review Comment:
Yes, you are right! Fixed.
--
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] moonming commented on pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
moonming commented on PR #7750:
URL: https://github.com/apache/apisix/pull/7750#issuecomment-1221441776
is this PR will fix https://github.com/apache/apisix/issues/7746?
--
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] spacewander commented on a diff in pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7750:
URL: https://github.com/apache/apisix/pull/7750#discussion_r950835195
##########
t/plugin/limit-count3.t:
##########
@@ -224,3 +224,64 @@ passed
}
--- response_body
[200,200]
+
+
+
+=== TEST 7: set another route with the same conf
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello1",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60
+ }
Review Comment:
> is the same conf mean the value of attr such as count or time_window all equal to another limit-count in other routes?
Yes.
--
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] spacewander commented on pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7750:
URL: https://github.com/apache/apisix/pull/7750#issuecomment-1221534868
> is this PR will fix #7746?
Yes
--
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] spacewander merged pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7750:
URL: https://github.com/apache/apisix/pull/7750
--
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] tzssangglass commented on a diff in pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7750:
URL: https://github.com/apache/apisix/pull/7750#discussion_r950708312
##########
t/plugin/limit-count3.t:
##########
@@ -224,3 +224,64 @@ passed
}
--- response_body
[200,200]
+
+
+
+=== TEST 7: set another route with the same conf
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello1",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60
+ }
Review Comment:
the conf of `limit-count` different form route 1
```
"limit-count": {
"count": 2,
"time_window": 61
}
```
is the same conf means the value of attr such as `count` or `time_window`?
--
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] nic-chen commented on pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
nic-chen commented on PR #7750:
URL: https://github.com/apache/apisix/pull/7750#issuecomment-1223475448
Hi @spacewander
I have a doubt. Will this change affect `global_rule`? Will each route count its limit when configuring a `limit-count` in a `global_rule` ? Thanks.
--
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] spacewander commented on pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7750:
URL: https://github.com/apache/apisix/pull/7750#issuecomment-1223542152
> Hi @spacewander I have a doubt. Will this change affect `global_rule`? Will each route count its limit when configuring a `limit-count` in a `global_rule` ? Thanks.
As all route shares the same global rule, they will use the same limit counter in this global rule.
--
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] tzssangglass commented on a diff in pull request #7750: fix(limit-count): different route with same conf should not share counter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7750:
URL: https://github.com/apache/apisix/pull/7750#discussion_r950708312
##########
t/plugin/limit-count3.t:
##########
@@ -224,3 +224,64 @@ passed
}
--- response_body
[200,200]
+
+
+
+=== TEST 7: set another route with the same conf
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello1",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60
+ }
Review Comment:
the conf of `limit-count` different form route 1
```
"limit-count": {
"count": 2,
"time_window": 61
}
```
is the same conf mean the value of attr such as `count` or `time_window` all equal to another `limit-count` in other routes?
--
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