feat: add additional option to control the default_conn_delay option of the limit-conn plugin #4604
Conversation
2、complete test cases
docs/en/latest/plugins/limit-conn.md
Outdated
| @@ -40,7 +40,7 @@ Limiting request concurrency plugin. | |||
| | conn | integer | required | | conn > 0 | the maximum number of concurrent requests allowed. Requests exceeding this ratio (and below `conn` + `burst`) will get delayed(the latency seconds is configured by `default_conn_delay`) to conform to this threshold. | | |||
| | burst | integer | required | | burst >= 0 | the number of excessive concurrent requests (or connections) allowed to be delayed. | | |||
| | default_conn_delay | number | required | | default_conn_delay > 0 | the latency seconds of request when concurrent requests exceeding `conn` but below (`conn` + `burst`). | | |||
| | delay_strict_mode | boolean | optional | false | [true,false] | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic. | | |||
| | delay_strict_mode | boolean | optional | false | [true,false] | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic. | | |||
There was a problem hiding this comment.
I think the option name is not so straightforward but I didn't come out with a more proper one :(
There was a problem hiding this comment.
What about only_use_default_delay?
There was a problem hiding this comment.
only_use_default_delay, use_default_conn_delay, apply_default_conn_delay, stint_default_conn_delay, Which of the four do you think is better, or do you have better suggestions?
There was a problem hiding this comment.
I vote for the first one. What about others' opinions?
There was a problem hiding this comment.
@tokers what do you think?
There was a problem hiding this comment.
Vote for the first one too, it's easy to understand.
There was a problem hiding this comment.
Okay, I will change it to this one.
What about the test case I mentioned above? Any suggestions? @spacewander @tokers
There was a problem hiding this comment.
You can take a look at the test in #4585
t/plugin/limit-conn2.t
Outdated
| --- request | ||
| GET /hello1 | ||
| --- grep_error_log eval | ||
| qr/request latency is/ |
There was a problem hiding this comment.
Need to check latency in this mode is nil.
There was a problem hiding this comment.
I don't know exactly what you mean,
hasn't grep_error_log_out already check that the latency is nil?
Or would you give some guidance on how to do it ?
There was a problem hiding this comment.
The grep pattern should be request latency is nil.
t/plugin/limit-conn2.t
Outdated
| --- grep_error_log eval | ||
| qr/request latency is/ | ||
| --- grep_error_log_out | ||
| request latency is |
There was a problem hiding this comment.
| request latency is | |
| request latency is nil |
What this PR does / why we need it:
feat: issues
ps:
I'm thinking how to do the unit test here
Because I cannot verify the logic of the third-party library lua-resty-limit-traffic, I can only verify whether nil is passed for the req_latency parameter, but what should I do with this unit test?
Can you give some advice?