Allow channel pool to refresh its channels periodically by tonytanger · Pull Request #805 · googleapis/gax-java · GitHub

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.

Codecov Report

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

@@             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 Files	Coverage Δ	Complexity Δ
...main/java/com/google/api/gax/grpc/ChannelPool.java	47.82% <100%> (+9.36%)	12 <2> (+5)	⬆️
.../google/api/gax/grpc/RefreshingManagedChannel.java	62.96% <62.96%> (ø)	7 <7> (?)
...oogle/api/gax/grpc/SafeShutdownManagedChannel.java	83.33% <83.33%> (ø)	9 <9> (?)
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	80.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.

Under what circumstances is the executor not available?

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.

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)

This comment seems stale

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

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)

…king shutdown

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

This comment seems stale

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

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

Codecov Report

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

@@             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 Files	Coverage Δ	Complexity Δ
...main/java/com/google/api/gax/grpc/ChannelPool.java	48.93% <100%> (+10.47%)	11 <4> (+4)	⬆️
.../google/api/gax/grpc/RefreshingManagedChannel.java	62.96% <62.96%> (ø)	7 <7> (?)
...oogle/api/gax/grpc/SafeShutdownManagedChannel.java	83.33% <83.33%> (ø)	9 <9> (?)
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	80.1% <94.44%> (+0.66%)	35 <2> (-1)	⬇️
.../java/com/google/api/gax/batching/BatcherImpl.java	97.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.

LGTM

one last nit, but ready merge after

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 16, 2019

igorbernstein2 suggested changes Oct 17, 2019

View reviewed changes

igorbernstein2 suggested changes Oct 29, 2019

View reviewed changes

igorbernstein2 reviewed Oct 29, 2019

View reviewed changes

Comment thread gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java Outdated

igorbernstein2 suggested changes Oct 31, 2019

View reviewed changes

igorbernstein2 suggested changes Nov 4, 2019

View reviewed changes

tonytanger added 12 commits November 13, 2019 13:13

add refresh capability to channelpool

ab31518

add tests

95cbd0b

fix minor issues

2a0c783

respond to comments and add synchronization to newcall and channel swap

60507ec

clean up concurrency test

161d439

Move refreshing logic to a wrapper class of managed channel

ac838a9

change channel pool test to remove reliance on timer

193fcc5

working solution

ddca27b

Move shutdown logic to sub class to safely perform to ensure non-bloc…

0101d69

…king shutdown

Add comments

dbc1a6e

Inline jitter calculation

b2f280b

Add comments and documentation

6b3968b

tonytanger force-pushed the refreshing-channel-pool branch from 69f4827 to 6b3968b Compare November 13, 2019 18:13

igorbernstein2 reviewed Nov 13, 2019

View reviewed changes

igorbernstein2 requested a review from vam-google November 13, 2019 19:50

vam-google reviewed Nov 13, 2019

View reviewed changes

Respond to comments

06a4db2

igorbernstein2 approved these changes Nov 19, 2019

View reviewed changes

Comment thread gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java Outdated

Comment thread gax-grpc/src/main/java/com/google/api/gax/grpc/RefreshingManagedChannel.java

tonytanger added 2 commits November 19, 2019 14:27

add comment on synchronization

5a3d4bd

simplified channel pool creation logic

ae03a15

vam-google approved these changes Nov 21, 2019

View reviewed changes

igorbernstein2 approved these changes Nov 21, 2019

View reviewed changes

Comment thread gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

Add for internal use only comment

69e903b

igorbernstein2 merged commit b059473 into googleapis:master Nov 21, 2019

chingor13 mentioned this pull request Dec 3, 2019

Release gax-java v1.51.0 #824

Merged

ejona86 mentioned this pull request May 29, 2020

Is RefreshingManagedChannel being used, or just an experiment? #1084

Closed

		// if executorService is available, create RefreshingManagedChannel that will get refreshed.
		// otherwise create with regular ManagedChannel

Allow channel pool to refresh its channels periodically by tonytanger · Pull Request #805 · googleapis/gax-java · GitHub

Uh oh!

Allow channel pool to refresh its channels periodically - #805

Allow channel pool to refresh its channels periodically#805igorbernstein2 merged 16 commits intogoogleapis:mastergoogleapis/gax-java:masterfrom tonytanger:refreshing-channel-pooltonytanger/gax-java:refreshing-channel-poolCopy head branch name to clipboard

Conversation

tonytanger commented Oct 16, 2019

Uh oh!

codecov Bot commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

tonytanger Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vam-google left a comment

Allow channel pool to refresh its channels periodically#805
igorbernstein2 merged 16 commits into
googleapis:masterfrom
tonytanger:refreshing-channel-pool

codecov Bot commented Oct 16, 2019 •
edited
Loading

codecov-io commented Nov 19, 2019 •
edited by codecov Bot
Loading