Allow channel pool to refresh its channels periodically #805

tonytanger · 2019-10-16T18:11:37Z

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 · 2019-10-16T18:14:46Z

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.

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelFactory.java

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

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPrimer.java

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

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

igorbernstein2 · 2019-10-30T22:08:49Z

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

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


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

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

igorbernstein2

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

igorbernstein2 · 2019-11-04T15:47:55Z

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

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


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)

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPrimer.java

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

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

gax-grpc/src/main/java/com/google/api/gax/grpc/SafeShutdownManagedChannel.java

…king shutdown

igorbernstein2

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

igorbernstein2 · 2019-11-13T14:42:31Z

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

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


This comment seems stale

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelFactory.java

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

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPrimer.java

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

gax-grpc/src/main/java/com/google/api/gax/grpc/SafeShutdownManagedChannel.java

gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest.java

vam-google

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

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

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

gax-grpc/src/main/java/com/google/api/gax/grpc/SafeShutdownManagedChannel.java

gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest.java

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

igorbernstein2

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

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

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

codecov-io · 2019-11-19T19:45:58Z

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.

vam-google

LGTM

igorbernstein2

one last nit, but ready merge after

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

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

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java Outdated Show resolved Hide resolved

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

gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java Outdated Show resolved Hide resolved

gax-grpc/src/main/java/com/google/api/gax/grpc/RefreshingManagedChannel.java Show resolved Hide resolved

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

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow channel pool to refresh its channels periodically #805

Allow channel pool to refresh its channels periodically #805

tonytanger commented Oct 16, 2019

codecov bot commented Oct 16, 2019 •

edited

igorbernstein2 Oct 30, 2019

tonytanger Nov 1, 2019

igorbernstein2 Nov 4, 2019

igorbernstein2 Nov 13, 2019

igorbernstein2 left a comment

igorbernstein2 Nov 4, 2019

igorbernstein2 left a comment

igorbernstein2 Nov 13, 2019

vam-google left a comment

igorbernstein2 left a comment

codecov-io commented Nov 19, 2019 •

edited by codecov bot

vam-google left a comment

igorbernstein2 left a comment

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

Allow channel pool to refresh its channels periodically #805

Allow channel pool to refresh its channels periodically #805

Conversation

tonytanger commented Oct 16, 2019

codecov bot commented Oct 16, 2019 • edited

Codecov Report

igorbernstein2 Oct 30, 2019

Choose a reason for hiding this comment

tonytanger Nov 1, 2019

Choose a reason for hiding this comment

igorbernstein2 Nov 4, 2019

Choose a reason for hiding this comment

igorbernstein2 Nov 13, 2019

Choose a reason for hiding this comment

igorbernstein2 left a comment

Choose a reason for hiding this comment

igorbernstein2 Nov 4, 2019

Choose a reason for hiding this comment

igorbernstein2 left a comment

Choose a reason for hiding this comment

igorbernstein2 Nov 13, 2019

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

igorbernstein2 left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 19, 2019 • edited by codecov bot

Codecov Report

vam-google left a comment

Choose a reason for hiding this comment

igorbernstein2 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2019 •

edited

codecov-io commented Nov 19, 2019 •

edited by codecov bot