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/03/09 08:36:15 UTC
[GitHub] [apisix] Gerrard-YNWA opened a new pull request #6562: feat: allow set timeout for proxy-mirror
Gerrard-YNWA opened a new pull request #6562:
URL: https://github.com/apache/apisix/pull/6562
### 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. -->
According to #6536 , add plugin attributes for proxy-mirror which allows to customize `proxy_connect_timeout`, `proxy_read_timeout` and `proxy_send_timeout` for mirrored sub-request.
### Pre-submission checklist:
<!--
Please follow the PR manners:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
7. Only reviewer can click "Resolve conversation" to mark the reviewer's review resolved
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
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] Gerrard-YNWA commented on a change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
Gerrard-YNWA commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r822512455
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -765,6 +765,11 @@ http {
}
{% end %}
+ {% if proxy_mirror_timeout then %}
+ {% for k, v in pairs(proxy_mirror_timeout) do %}
+ proxy_{* k *}_timeout {* v *}ms;
Review comment:
updated
--
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 #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6562:
URL: https://github.com/apache/apisix/pull/6562
--
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 change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r823255767
##########
File path: conf/config-default.yaml
##########
@@ -450,4 +450,8 @@ plugin_attr:
sequence_bits: 10 # each machine generates a maximum of (1 << sequence_bits) serial numbers per millisecond
data_machine_ttl: 30 # live time for data_machine in etcd (unit: second)
data_machine_interval: 10 # lease renewal interval in etcd (unit: second)
-
+ #proxy-mirror:
Review comment:
What about uncommenting them and setting a default timeout (60000)?
##########
File path: conf/config-default.yaml
##########
@@ -450,4 +450,8 @@ plugin_attr:
sequence_bits: 10 # each machine generates a maximum of (1 << sequence_bits) serial numbers per millisecond
data_machine_ttl: 30 # live time for data_machine in etcd (unit: second)
data_machine_interval: 10 # lease renewal interval in etcd (unit: second)
-
+ #proxy-mirror:
+ #timeout: # proxy timeout inmirrored sub-request (unit: milliseconds)
Review comment:
```suggestion
#timeout: # proxy timeout in mirrored sub-request (unit: milliseconds)
```
##########
File path: docs/zh/latest/plugins/proxy-mirror.md
##########
@@ -93,3 +93,22 @@ curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f1
```
这时该插件已被禁用。
+
+## 指定mirror子请求的超时时间
+
+我们可以在`conf/config.yaml`的`plugin_attr`中指定子请求的超时时间。这在连接复用的场景下mirror流量到一个非常慢的后端服务时非常有用。
Review comment:
```suggestion
我们可以在 `conf/config.yaml` 的 `plugin_attr` 中指定子请求的超时时间。这在连接复用的场景下 mirror 流量到一个非常慢的后端服务时非常有用。
```
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -765,6 +765,18 @@ http {
}
{% end %}
+
+ {% if proxy_mirror_timeouts then %}
+ {% if proxy_mirror_timeouts.connect then %}
+ proxy_connect_timeout {* proxy_mirror_timeouts.connect *}ms;
Review comment:
Would be better to put the time unit in the configuration value instead of hardcoded it.
##########
File path: docs/en/latest/plugins/proxy-mirror.md
##########
@@ -94,3 +94,22 @@ curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f1
```
The plugin has been disabled now.
+
+## Customize proxy timeout for mirrored sub-request
+
+We can customize proxy timeout for the mirrored sub-request in the `plugin_attr` of `conf/config.yaml`. This useful while mirroring traffic to a slow backend in a connection reused scenario.
Review comment:
```suggestion
We can customize proxy timeouts for the mirrored sub-request in the `plugin_attr` of `conf/config.yaml`. This is useful while mirroring traffic to a slow backend in a long connection scenario.
```
--
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] Gerrard-YNWA commented on a change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
Gerrard-YNWA commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r823372076
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -765,6 +765,18 @@ http {
}
{% end %}
+
+ {% if proxy_mirror_timeouts then %}
+ {% if proxy_mirror_timeouts.connect then %}
+ proxy_connect_timeout {* proxy_mirror_timeouts.connect *}ms;
Review comment:
How about keep the same to the nginx configuration and let users choose seconds or milliseconds by themselves
--
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] Gerrard-YNWA commented on a change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
Gerrard-YNWA commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r824469881
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -765,6 +765,18 @@ http {
}
{% end %}
+
+ {% if proxy_mirror_timeouts then %}
+ {% if proxy_mirror_timeouts.connect then %}
+ proxy_connect_timeout {* proxy_mirror_timeouts.connect *};
+ {% end %}
+ {% if proxy_mirror_timeouts.read then %}
+ proxy_read_timeout {* proxy_mirror_timeouts.read *};
+ {% end %}
+ {% if proxy_mirror_timeouts.send then %}
+ proxy_send_timeout {* proxy_mirror_timeouts.send *};
+ {% end %}
+ {% end %}
Review comment:
@soulbird No, I think this is a location level configuration and only works for /proxy_mirror internal request
--
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] tokers commented on a change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r822472758
##########
File path: apisix/cli/ops.lua
##########
@@ -513,6 +513,11 @@ Please modify "admin_key" in conf/config.yaml .
end
end
+ local proxy_mirror_timeout
Review comment:
```suggestion
local proxy_mirror_timeouts
```
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -765,6 +765,11 @@ http {
}
{% end %}
+ {% if proxy_mirror_timeout then %}
+ {% for k, v in pairs(proxy_mirror_timeout) do %}
+ proxy_{* k *}_timeout {* v *}ms;
Review comment:
This way we may pollute the nginx template, we may write some bad directive like: "proxy_foo_timeout".
--
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] Gerrard-YNWA commented on a change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
Gerrard-YNWA commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r823373949
##########
File path: conf/config-default.yaml
##########
@@ -450,4 +450,8 @@ plugin_attr:
sequence_bits: 10 # each machine generates a maximum of (1 << sequence_bits) serial numbers per millisecond
data_machine_ttl: 30 # live time for data_machine in etcd (unit: second)
data_machine_interval: 10 # lease renewal interval in etcd (unit: second)
-
+ #proxy-mirror:
Review comment:
Agree, 60000 is the default value of nginx, uncommenting and setting them can be clearer. What about respects to nginx's 60 in seconds instead of 60000 in milliseconds ?
--
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 change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
soulbird commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r824326754
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -765,6 +765,18 @@ http {
}
{% end %}
+
+ {% if proxy_mirror_timeouts then %}
+ {% if proxy_mirror_timeouts.connect then %}
+ proxy_connect_timeout {* proxy_mirror_timeouts.connect *};
+ {% end %}
+ {% if proxy_mirror_timeouts.read then %}
+ proxy_read_timeout {* proxy_mirror_timeouts.read *};
+ {% end %}
+ {% if proxy_mirror_timeouts.send then %}
+ proxy_send_timeout {* proxy_mirror_timeouts.send *};
+ {% end %}
+ {% end %}
Review comment:
These directives apply to all plugins, I don't think it's appropriate for you to do this. Maybe you can implement this functionality in your own plugin by `before_proxy`.
--
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 change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r824364495
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -765,6 +765,18 @@ http {
}
{% end %}
+
+ {% if proxy_mirror_timeouts then %}
+ {% if proxy_mirror_timeouts.connect then %}
+ proxy_connect_timeout {* proxy_mirror_timeouts.connect *};
+ {% end %}
+ {% if proxy_mirror_timeouts.read then %}
+ proxy_read_timeout {* proxy_mirror_timeouts.read *};
+ {% end %}
+ {% if proxy_mirror_timeouts.send then %}
+ proxy_send_timeout {* proxy_mirror_timeouts.send *};
+ {% end %}
+ {% end %}
Review comment:
No, there is a guard above:
```
{% if enabled_plugins["proxy-mirror"] then %}
```
--
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] Gerrard-YNWA commented on a change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
Gerrard-YNWA commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r822512331
##########
File path: apisix/cli/ops.lua
##########
@@ -513,6 +513,11 @@ Please modify "admin_key" in conf/config.yaml .
end
end
+ local proxy_mirror_timeout
Review comment:
updated
--
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] Gerrard-YNWA commented on a change in pull request #6562: feat: allow set timeout for proxy-mirror
Posted by GitBox <gi...@apache.org>.
Gerrard-YNWA commented on a change in pull request #6562:
URL: https://github.com/apache/apisix/pull/6562#discussion_r823374053
##########
File path: conf/config-default.yaml
##########
@@ -450,4 +450,8 @@ plugin_attr:
sequence_bits: 10 # each machine generates a maximum of (1 << sequence_bits) serial numbers per millisecond
data_machine_ttl: 30 # live time for data_machine in etcd (unit: second)
data_machine_interval: 10 # lease renewal interval in etcd (unit: second)
-
+ #proxy-mirror:
+ #timeout: # proxy timeout inmirrored sub-request (unit: milliseconds)
Review comment:
done
##########
File path: docs/zh/latest/plugins/proxy-mirror.md
##########
@@ -93,3 +93,22 @@ curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f1
```
这时该插件已被禁用。
+
+## 指定mirror子请求的超时时间
+
+我们可以在`conf/config.yaml`的`plugin_attr`中指定子请求的超时时间。这在连接复用的场景下mirror流量到一个非常慢的后端服务时非常有用。
Review comment:
done
##########
File path: docs/en/latest/plugins/proxy-mirror.md
##########
@@ -94,3 +94,22 @@ curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f1
```
The plugin has been disabled now.
+
+## Customize proxy timeout for mirrored sub-request
+
+We can customize proxy timeout for the mirrored sub-request in the `plugin_attr` of `conf/config.yaml`. This useful while mirroring traffic to a slow backend in a connection reused scenario.
Review comment:
done
--
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