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 2020/10/19 07:41:17 UTC
[GitHub] [apisix] membphis opened a new pull request #2465: bugfix(limit-conn): always necessary to save the data of the limit concurrency, and release the statistical status in the log phase.
membphis opened a new pull request #2465:
URL: https://github.com/apache/apisix/pull/2465
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
fix #2450
### Pre-submission checklist:
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-714567723
> I think the problem is solved. What made the fix won't work is just the way of test (for example, the upstream doesn't hold the connection).
I don't understand word "the upstream doesn't hold the connection"
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712767911
> > > > @LY-GO you can make a try with this PR
> > > > 2.i modify the code of limit-conn according to your repair.But the response code is 500
> > > > 
>
> > @LY-GO you can make a try with this PR
>
> I test the plugin,i find it don't work;now every request can success,even if number of request over concurrency limit.I just copy your code,overwrite a new plugin and reload the plugin
two suggesstions:
1.When you use limit-conn plugin,you must set default_conn_delay > 0, otherwise you will find assertion failed.
2.When you test limit-conn plugin,your upstream must set sleep time.Because concurrency is required at the same time .
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-714477649
I think the problem is solved. What made the fix won't work is just the way of test (for example, the upstream doesn't hold the connection).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO commented on pull request #2465: bugfix(limit-conn): always necessary to save the data of the limit concurrency, and release the statistical status in the log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712119756
> > @LY-GO you can make a try with this PR
> 2.i modify the code of limit-conn according to your repair.But the response code is 500
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2465: fix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#discussion_r539416362
##########
File path: t/plugin/limit-conn.t
##########
@@ -1207,9 +1207,9 @@ qr/limit key: consumer_jackroute&consumer\d+/
content_by_lua_block {
local plugin = require("apisix.plugins.limit-conn")
local ok, err = plugin.check_schema({
- conn = 1,
- default_conn_delay = 0.1,
- rejected_code = 503,
+ conn = 1,
+ default_conn_delay = 0.1,
+ rejected_code = 503,
Review comment:
old test cases have `space` at the end of line.
my editor helps me do the `slim`, this is fine.
##########
File path: t/plugin/limit-conn.t
##########
@@ -1225,3 +1225,61 @@ property "burst" is required
done
--- no_error_log
[error]
+
+
+
+=== TEST 32: enable plugin: conn=1
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-conn": {
+ "conn": 1,
+ "burst": 0,
+ "default_conn_delay": 0.3,
+ "rejected_code": 503,
+ "key": "remote_addr"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 33: hit route and should not be limited
+--- pipelined_requests eval
Review comment:
All requests are sent one after another, they are not concurrent.
So the request should never be limited for `"conn": 1,`.
In the old code, it will be limited due to this bug.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #2465: fix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #2465:
URL: https://github.com/apache/apisix/pull/2465
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2465: fix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-742276772
@moonming @tokers @nic-chen please take a look at this PR
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-713576882
@LY-GO please provide your plugin configuration. it maybe invalid.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO edited a comment on pull request #2465: bugfix(limit-conn): always necessary to save the data of the limit concurrency, and release the statistical status in the log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO edited a comment on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712118157
> @LY-GO you can make a try with this PR
two question:
1.now i have a requirement.iI need to add a key that it belonds to config of limit-conn-plugin .I just modify the define of key?
<img width="856" alt="key" src="https://user-images.githubusercontent.com/67543312/96444774-73ce3480-1241-11eb-8e95-f5a8b26a1ad8.png">
2.i modify the code of limit-conn according to your repair.But the response code is 500
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-713422763
@LY-GO
> I test the plugin,i find it don't work;now every request can success,even if number of request over concurrency limit.I just copy your code,overwrite a new plugin and reload the plugin
> 2.When you test limit-conn plugin,your upstream must set sleep time.Because concurrency is required at the same time .
Is the note 2 caused the plugin didn't work in your test? Does it work after you set sleep time?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO removed a comment on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO removed a comment on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712621681
> @LY-GO you can make a try with this PR
I test the plugin,i find it don't work;now every request can success,even if number of request over concurrency limit.I just copy your code,overwrite a new plugin and reload the plugin.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712621681
> @LY-GO you can make a try with this PR
I test the plugin,i find it don't work;now every request can success,even if number of request over concurrency limit.I just copy your code,overwrite a new plugin and reload the plugin.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-740451408
@tokers @moonming @spacewander I have fixed the conflict, you can take a look
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712625105
> > > @LY-GO you can make a try with this PR
> > > 2.i modify the code of limit-conn according to your repair.But the response code is 500
> > > 
> @LY-GO you can make a try with this PR
I test the plugin,i find it don't work;now every request can success,even if number of request over concurrency limit.I just copy your code,overwrite a new plugin and reload the plugin
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO edited a comment on pull request #2465: bugfix(limit-conn): always necessary to save the data of the limit concurrency, and release the statistical status in the log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO edited a comment on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712118157
> @LY-GO you can make a try with this PR
two question:
1.now i have a requirement.iI need to add a key that it belonds to config of limit-conn-plugin .I just modify the define of key?
<img width="856" alt="key" src="https://user-images.githubusercontent.com/67543312/96444774-73ce3480-1241-11eb-8e95-f5a8b26a1ad8.png">
2.i modify the code of limit-conn according to your repair.But the response code is 500
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis edited a comment on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
membphis edited a comment on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-713576882
@LY-GO please provide your plugin configuration. it maybe invalid.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2465: bugfix(limit-conn): always necessary to save the data of the limit concurrency, and release the statistical status in the log phase.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-711793684
@LY-GO you can make a try with this PR
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712577029
@LY-GO
I think the 500 may be related with #2472
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2465: fix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#discussion_r539369129
##########
File path: t/plugin/limit-conn.t
##########
@@ -1207,9 +1207,9 @@ qr/limit key: consumer_jackroute&consumer\d+/
content_by_lua_block {
local plugin = require("apisix.plugins.limit-conn")
local ok, err = plugin.check_schema({
- conn = 1,
- default_conn_delay = 0.1,
- rejected_code = 503,
+ conn = 1,
+ default_conn_delay = 0.1,
+ rejected_code = 503,
Review comment:
why modify this test case？
##########
File path: t/plugin/limit-conn.t
##########
@@ -1225,3 +1225,61 @@ property "burst" is required
done
--- no_error_log
[error]
+
+
+
+=== TEST 32: enable plugin: conn=1
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-conn": {
+ "conn": 1,
+ "burst": 0,
+ "default_conn_delay": 0.3,
+ "rejected_code": 503,
+ "key": "remote_addr"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 33: hit route and should not be limited
+--- pipelined_requests eval
Review comment:
Shouldn't the request be rejected here? I did not understand
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO commented on pull request #2465: bugfix(limit-conn): always necessary to save the data of the limit concurrency, and release the statistical status in the log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-712118157
> @LY-GO you can make a try with this PR
two question:
1.now i have a requirement.iI need to add a key that it belonds to config of limit-conn-plugin .I just modify the define of key?
<img width="856" alt="key" src="https://user-images.githubusercontent.com/67543312/96444774-73ce3480-1241-11eb-8e95-f5a8b26a1ad8.png">
2.i modify the code of limit-conn according to your repair.But the response code is 500
![Uploading 披露工.png…]()
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [apisix] LY-GO commented on pull request #2465: bugfix(limit-conn): always save the data of the limit object, and release it in log phase.
Posted by GitBox <gi...@apache.org>.
LY-GO commented on pull request #2465:
URL: https://github.com/apache/apisix/pull/2465#issuecomment-713645906
> @LY-GO
>
> > I test the plugin,i find it don't work;now every request can success,even if number of request over concurrency limit.I just copy your code,overwrite a new plugin and reload the plugin
>
> > 2.When you test limit-conn plugin,your upstream must set sleep time.Because concurrency is required at the same time .
>
> Is the note 2 caused the plugin didn't work in your test? Does it work after you set sleep time?
When you set sleep time in your procedure,you can use jmeter to test plugin and the plugin can work successfully.
----------------------------------------------------------------
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:
users@infra.apache.org