Change http2MaxRequests setting to maxRequests
#1871
Conversation
kailun-qin
requested review from
dcberg,
duderino,
linsun,
louiscryan,
nrjpoddar and
smawson
as code owners
|
😊 Welcome @kailun-qin! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, code of conduct, and contributing guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
google-cla
bot
added
the
cla: yes
istio-testing
added
the
size/S
|
@kailun-qin: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| @@ -607,7 +607,7 @@ message ConnectionPoolSettings { | |||
| int32 http1_max_pending_requests = 1; | |||
|
|
|||
| // Maximum number of requests to a backend. Default 2^32-1. | |||
| int32 http2_max_requests = 2; | |||
| int32 max_requests = 2; | |||
There was a problem hiding this comment.
We can not change these names now as it will break existing deployments. We have to follow the deprecation process
There was a problem hiding this comment.
I assume this and http1_max_pending_requests will also need to be deprecated, new field added - and some tooling in istioctl before we even consider removing the old field and behavior.
An alternative is to just change the documentation - "Istio may upgrade HTTP/1.1 requests to HTTP/2 internally, this setting will cover both HTTP/1.1 and HTTP/2 requests". That's what we plan to do with BTS AFAIK, everything including TCP will be over H2.
If we add a new field - we should also consider BTS, and move it up, since TCP will also be treated as a long-lived H2.
There was a problem hiding this comment.
I was thinking about update the doc as @costinm mentioned. IMO, that is better - it is not worth the churn to add a new field and retain the old field forever.
There was a problem hiding this comment.
http1_max_pending_requests might be slightly different since it involves extra settings to cover behaviors of H2.
Anyway, marking BTS related issues here: istio/istio#24540, istio/istio#25713.
| @@ -607,7 +607,7 @@ message ConnectionPoolSettings { | |||
| int32 http1_max_pending_requests = 1; | |||
|
|
|||
| // Maximum number of requests to a backend. Default 2^32-1. | |||
| int32 http2_max_requests = 2; | |||
| int32 max_requests = 2; | |||
The combination of H1 and H2 conn_pool implementations makes `max_requests` to work for both H1 and H2 in Envoy proxy (envoyproxy/envoy#9215). In accordance with this change, this patch updated the field `http2MaxRequests` of `ConnectionPoolSettings.HTTPSettings` in DestinationRule API to `maxRequests` to resolve the outdated H1 only implications/misunderstandings for the field. Addresses istio/istio#27473 Signed-off-by: Kailun Qin <[email protected]>
istio-testing
added
the
needs-rebase
|
@kailun-qin: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
howardjohn
removed
the
cla: yes
|
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2021-02-03. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions. Created by the issue and PR lifecycle manager. |
istio-policy-bot
added
the
lifecycle/automatically-closed
The combination of H1 and H2 conn_pool implementations makes
max_requeststowork for both H1 and H2 in Envoy proxy
(envoyproxy/envoy#9215).
In accordance with this change, this patch updated the field
http2MaxRequestsof
ConnectionPoolSettings.HTTPSettingsin DestinationRule API tomaxRequeststo resolve the outdated H1 only implications/misunderstandings for the field.
Addresses istio/istio#27473
Signed-off-by: Kailun Qin [email protected]