-
Notifications
You must be signed in to change notification settings - Fork 4.9k
core: unify uint32_t in connection pool clients and max_request_per_connection #39275
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
core: unify uint32_t in connection pool clients and max_request_per_connection #39275
Conversation
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
/assign-from @envoyproxy/envoy-maintainers |
@envoyproxy/envoy-maintainers assignee is @jmarantz |
Signed-off-by: Boteng Yao <boteng@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm mostly; couple questions.
ActiveClient(HttpConnPoolImplBase& parent, uint64_t lifetime_stream_limit, | ||
uint64_t effective_concurrent_stream_limit, | ||
uint64_t configured_concurrent_stream_limit, | ||
ActiveClient(HttpConnPoolImplBase& parent, uint32_t lifetime_stream_limit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does 'lifetime' mean here? I'm just wondering if we are counting a potentially huge number of events over the lifetime of a server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the upstream's connection's life time to the backends, and it could be large, but it is not the lifetime of a server.
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
max_requests_per_connection
isuint32Value
in the http options and cluster configs, so no need to useuint64_t
uint64_t
for any limits and the baseEnvoy::ConnectionPool::ActiveClient
are alluint32_t
.Commit Message:
Additional Description:
Risk Level: low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features: