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

Create a executorService with a separate pool of threads for channelpool #836

Conversation

tonytanger
Copy link
Contributor

@tonytanger tonytanger commented Dec 18, 2019

Refreshing ChannelPool shares the executorService with gRPC. This could lead to undesirable behaviour. If it takes a while (a few seconds) to refresh a channel and many channels are being refreshed at the same time, it could occupy all the threads in the executorService and starve gRPC from using them.

Proposed is to create a separate executorService with its own pools of threads. It shouldn't need many threads to refresh the channels.

@googlebot googlebot added the cla: yes label Dec 18, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 18, 2019

Codecov Report

Merging #836 into master will decrease coverage by 0.11%.
The diff coverage is 52.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #836      +/-   ##
===========================================
- Coverage     78.72%   78.6%   -0.12%     
- Complexity     1146    1149       +3     
===========================================
  Files           202     202              
  Lines          5099    5113      +14     
  Branches        405     410       +5     
===========================================
+ Hits           4014    4019       +5     
- Misses          912     921       +9     
  Partials        173     173
Impacted Files Coverage Δ Complexity Δ
...api/gax/grpc/InstantiatingGrpcChannelProvider.java 79.89% <100%> (-0.22%) 35 <0> (ø)
...main/java/com/google/api/gax/grpc/ChannelPool.java 47.61% <50%> (-1.32%) 14 <2> (+3)

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 68ae22d...877bfa8. Read the comment docs.

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

LGTM!

@igorbernstein2 igorbernstein2 merged commit f18673a into googleapis:master Jan 6, 2020
1 of 2 checks passed
@kolea2 kolea2 mentioned this pull request Jan 9, 2020
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

3 participants