Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow channel pool to refresh its channels periodically #805

Merged
merged 16 commits into from Nov 21, 2019

Conversation

tonytanger
Copy link
Contributor

@tonytanger tonytanger commented Oct 16, 2019

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.

@googlebot googlebot added the cla: yes label Oct 16, 2019
@codecov
Copy link

@codecov codecov bot commented Oct 16, 2019

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

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

@igorbernstein2 igorbernstein2 Oct 31, 2019

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

@tonytanger tonytanger Nov 1, 2019

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.

Copy link
Collaborator

@igorbernstein2 igorbernstein2 Nov 4, 2019

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
Collaborator

@igorbernstein2 igorbernstein2 Nov 13, 2019

Choose a reason for hiding this comment

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

This comment seems stale

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

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

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

@igorbernstein2 igorbernstein2 Nov 4, 2019

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)

@tonytanger tonytanger force-pushed the refreshing-channel-pool branch from 69f4827 to 6b3968b Compare Nov 13, 2019
Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

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

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

@igorbernstein2 igorbernstein2 Nov 13, 2019

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 requested a review from vam-google Nov 13, 2019
Copy link
Contributor

@vam-google vam-google left a comment

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

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

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

@codecov-io
Copy link

@codecov-io codecov-io commented Nov 19, 2019

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

Copy link
Contributor

@vam-google vam-google left a comment

LGTM

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

one last nit, but ready merge after

@igorbernstein2 igorbernstein2 merged commit b059473 into googleapis:master Nov 21, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants