This repository has been archived by the owner on Sep 26, 2023. It is now read-only.
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.
Allow channel pool to refresh its channels periodically #805
Allow channel pool to refresh its channels periodically #805
Changes from 12 commits
ab31518
95cbd0b
2a0c783
60507ec
161d439
ac838a9
193fcc5
ddca27b
0101d69
dbc1a6e
b2f280b
6b3968b
06a4db2
5a3d4bd
ae03a15
69e903b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Under what circumstances is the executor not available?
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.
the executor is not available when we want to create a channel pool that's non-refreshing. InstantiatingGrpcChannelProvider creates a channel pool with a null executorService if channelPrimer is not set. See https://github.com/googleapis/gax-java/pull/805/files#diff-25ba434a47d99bcc0b998965a298661cR208-R211.
I suppose we could make every channel pool refresh regardless of whether channelPrimer is set. But that means the channel will refresh at whatever rate we specify in RefreshingManagedChannel and not when the server resets. I'm not sure if this is a desired outcome if the clients don't specify anything.
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.
I think it's ok to make the behavior configureable and off by default. But i don't think we should make it dependent on the presence of an executor service. It's a bit surprising. Maybe add 2 factory methods:
create(int poolSize, ChannelFactory channelFactory)
createRefreshing(int poolSize, ChannelFactory channelFactory, ScheduledExecutorService executor)
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.
This comment seems stale