Configure pooled connection lifetime in client factory #1862
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JamesNK
requested review from
BrennanConroy,
brunolins16,
captainsafia and
halter73
SamGuoMsft
reviewed
Aug 26, 2022
| // A channel is created once per client, lives forever, and the primary handler never changes. | ||
| // It's possible that long lived connections cause the client to miss out on DNS changes. | ||
| // Replicate the core benefit of a handler lifetime (periodic connection recreation) | ||
| // by setting PooledConnectionLifetime to handler lifetime. |
There was a problem hiding this comment.
Thanks @JamesNK to address this issue based on our offline discussion. Leave few comments here:
- Since we cache channel per typed/named client (no matter the client is transient or singleton), and use PooledConnectionLifetime to control handler lifetime, we are only using httpclientfactory as configuration API, the httpclientfactory handler pool is redundant in this scenario. I think we should work with runtime team to make httpclientfactory just as configuration API.
- With caching channel and use PooledConnectionLifetime, would you mind to share more details on why we DI GrpcClientFactory as transient rather than singleton (same as DefatultHttpClientFactory)?
services.TryAddTransient<GrpcClientFactory, DefaultGrpcClientFactory>();
There was a problem hiding this comment.
With caching channel and use PooledConnectionLifetime, would you mind to share more details on why we DI GrpcClientFactory as transient rather than singleton (same as DefatultHttpClientFactory)?
services.TryAddTransient<GrpcClientFactory, DefaultGrpcClientFactory>();
So the current IServiceProvider scope is injected into the factory.
JamesNK
force-pushed
the
jamesnk/factory-pooled-connection-lifetime
branch
from
6605a84 to
e994f23
Compare
captainsafia
approved these changes
Aug 29, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1861