This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Conversation

tonytanger

This address #804

ChannelPool is modified to managed the lifecycle of its channels, including the initial creation and periodically creating new channels and replacing old channels.

ChannelPrimer will be implemented by the clients to choose what they want to do during channel creation to prime the channel.

InstantiatingGrpcChannelProvider adds the ability to create ChannelPools that will refresh its channels. If ChannelPrimer is not provided, ChannelPool will not refresh its connection, and the functionality will be exactly the same as before.

@googlebotgooglebot added the cla: yesThis human has signed the Contributor License Agreement.label Oct 16, 2019
@codecov

Codecov Report

Merging #805 into master will decrease coverage by 0.07%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #805      +/-   ##
============================================
- Coverage     78.76%   78.68%   -0.08%     
- Complexity     1126     1146      +20     
============================================
  Files           200      202       +2     
  Lines          4983     5091     +108     
  Branches        398      405       +7     
============================================
+ Hits           3925     4006      +81     
- Misses          888      912      +24     
- Partials        170      173       +3
Impacted FilesCoverage ΔComplexity Δ
...main/java/com/google/api/gax/grpc/ChannelPool.java47.82% <100%> (+9.36%)12 <2> (+5)⬆️
.../google/api/gax/grpc/RefreshingManagedChannel.java62.96% <62.96%> (ø)7 <7> (?)
...oogle/api/gax/grpc/SafeShutdownManagedChannel.java83.33% <83.33%> (ø)9 <9> (?)
...api/gax/grpc/InstantiatingGrpcChannelProvider.java80.1% <94.44%> (+0.66%)35 <2> (-1)⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fad1d50...5a3d4bd. Read the comment docs.

igorbernstein2
igorbernstein2 suggested changes Oct 17, 2019
View reviewed changes
igorbernstein2
igorbernstein2 suggested changes Oct 29, 2019
View reviewed changes
igorbernstein2
igorbernstein2 suggested changes Oct 31, 2019
View reviewed changes
gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java Outdated
Comment on lines 64 to 88
// if executorService is available, create RefreshingManagedChannel that will get refreshed.
// otherwise create with regular ManagedChannel
Copy link
Contributor

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?

Copy link
Contributor Author

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://.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.

Copy link
Contributor

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems stale

igorbernstein2
igorbernstein2 suggested changes Nov 4, 2019
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, I think all race conditions have been addressed. Most comments are cosmetic

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java Outdated
Comment on lines 64 to 88
// if executorService is available, create RefreshingManagedChannel that will get refreshed.
// otherwise create with regular ManagedChannel
Copy link
Contributor

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)

@tonytangertonytanger force-pushed the refreshing-channel-pool branch from 69f4827 to 6b3968b Compare November 13, 2019 18:13
igorbernstein2
igorbernstein2 reviewed Nov 13, 2019
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good! I have a few more comments on style though:

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java Outdated
Comment on lines 64 to 88
// if executorService is available, create RefreshingManagedChannel that will get refreshed.
// otherwise create with regular ManagedChannel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems stale

vam-google
vam-google reviewed Nov 13, 2019
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember most of the context here, unfortunately. Just a few sudo-nitpicking comments from me. Overall looks good.

igorbernstein2
igorbernstein2 approved these changes Nov 19, 2019
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @vam-google, if this looks good to you, I think we can merge

@codecov-io

Codecov Report

Merging #805 into master will decrease coverage by 0.09%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #805     +/-   ##
===========================================
- Coverage     78.76%   78.67%   -0.1%     
- Complexity     1126     1144     +18     
===========================================
  Files           200      202      +2     
  Lines          4983     5092    +109     
  Branches        398      404      +6     
===========================================
+ Hits           3925     4006     +81     
- Misses          888      912     +24     
- Partials        170      174      +4
Impacted FilesCoverage ΔComplexity Δ
...main/java/com/google/api/gax/grpc/ChannelPool.java48.93% <100%> (+10.47%)11 <4> (+4)⬆️
.../google/api/gax/grpc/RefreshingManagedChannel.java62.96% <62.96%> (ø)7 <7> (?)
...oogle/api/gax/grpc/SafeShutdownManagedChannel.java83.33% <83.33%> (ø)9 <9> (?)
...api/gax/grpc/InstantiatingGrpcChannelProvider.java80.1% <94.44%> (+0.66%)35 <2> (-1)⬇️
.../java/com/google/api/gax/batching/BatcherImpl.java97.33% <0%> (-0.67%)15% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fad1d50...69e903b. Read the comment docs.

vam-google
vam-google approved these changes Nov 21, 2019
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

igorbernstein2
igorbernstein2 approved these changes Nov 21, 2019
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last nit, but ready merge after

@igorbernstein2igorbernstein2 merged commit b059473 into googleapis:master Nov 21, 2019
@chingor13chingor13 mentioned this pull request Dec 3, 2019
Merged
@ejona86ejona86 mentioned this pull request May 29, 2020
Closed
Sign up for free to subscribe to this conversation on . Already have an account? Sign in.

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

@vam-google vam-google vam-google approved these changes

No one assigned
cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.