feat: dynamic channel pool scaled by number of outstanding request #1569
Conversation
…nelPool. The eventual goal is to allow channel pool to safely add remove channels. The code has been refactored roughly as: - SafeShutdownManagedChannel is now ChannelPool.Entry and ReleasingClientCall - RefreshingManagedChannel has been merged into ChannelPool as a pair of functions scheduleNextRefresh & refresh
google-cla bot added the cla: yes 
igorbernstein2 force-pushed the dynamic-channel-pool branch from 7f7af96 to 6a8d3eb Compare 
| Let me know once this gets out of the draft status for review. |
….java Co-authored-by: Chanseok Oh <chanseok@google.com>
….java Co-authored-by: Chanseok Oh <chanseok@google.com>
Co-authored-by: Chanseok Oh <chanseok@google.com>
igorbernstein2 force-pushed the dynamic-channel-pool branch from 5b816d6 to db3b3f4 Compare igorbernstein2 force-pushed the dynamic-channel-pool branch from db3b3f4 to 4ebbebe Compare igorbernstein2 changed the title 
| Ok, I rebased on the channel refactor, this should be ready for review |
chanseokoh left a comment
chanseokoh left a comment There was a problem hiding this comment.
Still reviewing, but releasing some comments early.
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/ChannelPool.java Outdated Show resolved Hide resolved
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/ChannelPool.java Outdated Show resolved Hide resolved
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/ChannelPool.java Outdated Show resolved Hide resolved
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/ChannelPool.java Outdated Show resolved Hide resolved
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/ChannelPool.java Outdated Show resolved Hide resolved
| | ||
| // Number of channels if each channel operated at minimum capacity | ||
| int maxChannels = | ||
| (int) Math.ceil(actualOutstandingRpcs / (double) settings.getMinRpcsPerChannel()); |
There was a problem hiding this comment.
I see by default getMinRpcsPerChannel() returns 0, so we are diving by 0. That doesn't fail, but the result is
- if
actualOutstandingRpcs== 0, thenmaxChannelis 0. - if
actualOutstandingRpcs!= 0, then it's 2147483647.
This is still OK, since you're bounding maxChannels below. However, the division-by-0 computation is not trivial to verify and the computation result of 2147483647 seems atypical. The code gives the impression that the author probably failed to anticipate possible division by 0. At least I want to add a comment like "getMinRpcsPerChannel() can return 0, but division by 0 shouldn't cause a problem."
gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPoolSettings.java Outdated Show resolved Hide resolved
| public abstract int getMinRpcsPerChannel(); | ||
| | ||
| /** | ||
| * Threshold to start scaling up the channel pool. |
There was a problem hiding this comment.
Is it worth mentioning that setting there's a breaking point at 100 regarding performance (if I understand it correctly)? For example, if set to too high value, a lot of RPCs may be pending because gRPC's limit of 100 concurrent RPCs per channel?
gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPoolSettings.java Outdated Show resolved Hide resolved
gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Show resolved Hide resolved
gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest.java Outdated Show resolved Hide resolved
# Conflicts: # gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
| Sorry for the delay. I finally found time to address your feedback. PTAL |
| BTW, tests are failing. |
| fixed .. it was a stray import that leaked from the merge with main |
chanseokoh left a comment
chanseokoh left a comment There was a problem hiding this comment.
LGTM, pending a few minor comments. (For quick search, please check all previous comments that have not been marked resolved and closed.)
| Ok I think everything has been addressed. If you are ok with this, please merge it when you have a chance. |
| SonarCloud Quality Gate failed. |
| @igorbernstein2 Do any of the code smells detected by SonarCloud make sense? |
3 participants
This introduces the ability for gRPC channel pools to resize based on the number of outstanding RPCs.
Previously the pool was statically sized when the client was instantiated. It should automate the process described here: https://cloud.google.com/bigtable/docs/configure-connection-pools
The feature added in this PR are opt-in and shouldn't change or break any existing behavior. To enable dynamic channel pools, google clients or customers would have to set ChannelPoolSettings in StubSettings.
This PR adds the following features:
Please note that autoscaling will not play well with gcp clients that use affinity (pubsub & spanner). If the 2 features need to be combined, then we have a couple of options: we can use consistent hashing or maybe a proper api for affinity can be added where the lifecycle if affinity can be leased