Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(limit-conn): always save the data of the limit object, and release it in log phase. #2465

Merged
merged 1 commit into from
Dec 10, 2020
Merged

Conversation

membphis
Copy link
Member

@membphis membphis commented Oct 19, 2020

What this PR does / why we need it:

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?

@membphis
Copy link
Member Author

@LY-GO you can make a try with this PR

@LY-GO
Copy link

LY-GO commented Oct 19, 2020

@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?

key

2.i modify the code of limit-conn according to your repair.But the response code is 500

@LY-GO
Copy link

LY-GO commented Oct 19, 2020

@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
code

@membphis membphis changed the title bugfix(limit-conn): always necessary to save the data of the limit concurrency, and release the statistical status in the log phase. bugfix(limit-conn): always save the data of the limit object, and release it in log phase. Oct 19, 2020
@spacewander
Copy link
Member

@LY-GO
I think the 500 may be related with #2472

@LY-GO
Copy link

LY-GO commented Oct 20, 2020

@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
code

@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

@LY-GO
Copy link

LY-GO commented Oct 20, 2020

@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
code

@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 .

@spacewander
Copy link
Member

@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?

@membphis
Copy link
Member Author

membphis commented Oct 21, 2020

@LY-GO please provide your plugin configuration. it maybe invalid.

@LY-GO
Copy link

LY-GO commented Oct 21, 2020

@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.

@spacewander
Copy link
Member

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).

@LY-GO
Copy link

LY-GO commented Oct 22, 2020

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"

…nd release the statistical status in the log phase.

fix #2450
@membphis
Copy link
Member Author

membphis commented Dec 8, 2020

@tokers @moonming @spacewander I have fixed the conflict, you can take a look

@spacewander spacewander changed the title bugfix(limit-conn): always save the data of the limit object, and release it in log phase. fix(limit-conn): always save the data of the limit object, and release it in log phase. Dec 9, 2020
rejected_code = 503,
conn = 1,
default_conn_delay = 0.1,
rejected_code = 503,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why modify this test case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old test cases have space at the end of line.

my editor helps me do the slim, this is fine.

image



=== TEST 33: hit route and should not be limited
--- pipelined_requests eval
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the request be rejected here? I did not understand

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@spacewander spacewander added this to the 2.2 milestone Dec 10, 2020
@membphis
Copy link
Member Author

@moonming @tokers @nic-chen please take a look at this PR

@spacewander spacewander merged commit a670ea2 into apache:master Dec 10, 2020
@membphis membphis deleted the bug-limit-conn branch December 10, 2020 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: limit-conn
4 participants