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

circuit breaking: Change max_requests to work for both HTTP/1 and HTTP/2 #9215

Closed
tonya11en opened this issue Dec 4, 2019 · 3 comments · Fixed by #9668
Closed

circuit breaking: Change max_requests to work for both HTTP/1 and HTTP/2 #9215

tonya11en opened this issue Dec 4, 2019 · 3 comments · Fixed by #9668
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Milestone

Comments

@tonya11en
Copy link
Member

The current way circuit breakers are used to limit outstanding requests involves using max_requests for HTTP/2 and max_connections for HTTP/1. This is unintuitive and and requires special handling for code that requires knowledge of the number of outstanding requests (such as retry budgets).

I'd like to propose altering the way we increment outstanding requests in the per-cluster resource managers to track requests between both HTTPs. This would allow users to simply set max_requests circuit breaker values and be agnostic to the HTTP version.

This is particularly risky because it's a subtle change in behavior for widely used parameters. Hiding this behavior behind a feature flag would certainly be necessary, but I'll defer to maintainers/community on the best approach to making a change like this (if it's desired).

@mattklein123 mattklein123 added enhancement Feature requests. Not bugs or questions. help wanted Needs help! labels Dec 4, 2019
@mattklein123 mattklein123 added this to the 1.13.0 milestone Dec 4, 2019
@mattklein123
Copy link
Member

+1 we should fix this. It has been a longstanding source of confusion. We definitely need to runtime guard this and probably initially default to off. cc @alyssawilk

@mattklein123
Copy link
Member

Also related to #7403 in which we would optimally like to unify the H1 and H2 connection pool implementations as much as possible.

@oschaaf
Copy link
Member

oschaaf commented Dec 15, 2019

Drive-by comment:

This makes sense to me; a related idea after reading this:

The h2 pool implementation currently implies this, but once it supports multiple connections, perhaps a new max_requests_per_connection would make sense?
This could then translate into a (hypothetical) max request-pipelining depth for a semi-equivalent on h1.

kailun-qin added a commit to kailun-qin/api that referenced this issue Feb 3, 2021
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants