feat: dynamic channel pool scaled by number of outstanding request #1569

igorbernstein2 · 2021-11-22T21:00:44Z

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:

Each channel in the pool tracks the maximum number of concurrent RPC it has seen
every minute, a background thread loops through all of the channels and averages the max of each channel.
the average is compared to a threshold range. If the number falls outside of the range, the pool is resized to be in the middle of the range

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

…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

…nManagedChannel

chanseokoh · 2021-11-23T15:57:22Z

Let me know once this gets out of the draft status for review.

…or-channel-pool

….java Co-authored-by: Chanseok Oh <[email protected]>

Co-authored-by: Chanseok Oh <[email protected]>

igorbernstein2 · 2022-01-07T17:36:59Z

Ok, I rebased on the channel refactor, this should be ready for review

chanseokoh

Still reviewing, but releasing some comments early.

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

chanseokoh · 2022-01-11T22:23:53Z

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

+
+    // Number of channels if each channel operated at minimum capacity
+    int maxChannels =
+        (int) Math.ceil(actualOutstandingRpcs / (double) settings.getMinRpcsPerChannel());


I see by default getMinRpcsPerChannel() returns 0, so we are diving by 0. That doesn't fail, but the result is

if actualOutstandingRpcs == 0, then maxChannel is 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

chanseokoh · 2022-01-11T22:51:49Z

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

+  public abstract int getMinRpcsPerChannel();
+
+  /**
+   * Threshold to start scaling up the channel pool.


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

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

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

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

igorbernstein2 · 2022-02-11T20:49:50Z

Sorry for the delay. I finally found time to address your feedback. PTAL

chanseokoh · 2022-02-11T23:13:43Z

BTW, tests are failing.

igorbernstein2 · 2022-02-14T18:50:30Z

fixed .. it was a stray import that leaked from the merge with main

chanseokoh

LGTM, pending a few minor comments. (For quick search, please check all previous comments that have not been marked resolved and closed.)

igorbernstein2 · 2022-02-15T16:29:35Z

Ok I think everything has been addressed. If you are ok with this, please merge it when you have a chance.

sonarqubecloud · 2022-02-15T16:31:13Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
22 Code Smells

74.4% Coverage
0.0% Duplication

meltsufin · 2022-02-15T16:52:59Z

@igorbernstein2 Do any of the code smells detected by SonarCloud make sense?

igorbernstein2 added 3 commits November 22, 2021 15:48

migrate SafeShutdownManagedChannel tests

7e02e4a

migrate old tests and remove RefreshingManagedChannel and SafeShutdow…

50735bd

…nManagedChannel

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 22, 2021

fix test

3a501e0

igorbernstein2 force-pushed the dynamic-channel-pool branch from 7f7af96 to 6a8d3eb Compare November 22, 2021 22:02

vam-google requested review from meltsufin and chanseokoh November 22, 2021 23:59

chanseokoh and others added 15 commits November 30, 2021 17:30

Merge branch 'main' into refactor-channel-pool

321eb63

Merge branch 'main' into refactor-channel-pool

2e00f63

address feedback

6cb0964

Merge remote-tracking branch 'igor/refactor-channel-pool' into refact…

ec33d83

…or-channel-pool

Merge branch 'main' into refactor-channel-pool

8be5066

fix race condition on refresh()

8f6e39e

fix warnings in test

c93daf4

Merge remote-tracking branch 'origin/main' into refactor-channel-pool

a386cf2

Merge remote-tracking branch 'igor/refactor-channel-pool' into refact…

9dfa4e9

…or-channel-pool

Update gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest…

0062b0f

….java Co-authored-by: Chanseok Oh <[email protected]>

Update gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest…

cc0b507

….java Co-authored-by: Chanseok Oh <[email protected]>

handle race condition

7bf8b6d

Merge branch 'main' into refactor-channel-pool

4858e76

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

d2d7830

Co-authored-by: Chanseok Oh <[email protected]>

Merge branch 'main' into refactor-channel-pool

464bb13

igorbernstein2 force-pushed the dynamic-channel-pool branch from 5b816d6 to db3b3f4 Compare January 7, 2022 17:06

introduce dynamic channel pool

4ebbebe

igorbernstein2 force-pushed the dynamic-channel-pool branch from db3b3f4 to 4ebbebe Compare January 7, 2022 17:26

igorbernstein2 added 3 commits January 7, 2022 12:30

Merge remote-tracking branch 'origin/main' into dynamic-channel-pool

7cc62d0

fix test after broken merge

5f65a37

format

10c0cf0

igorbernstein2 changed the title ~~feat: dynamic channel pool scaled by number of outstanding request (WIP)~~ feat: dynamic channel pool scaled by number of outstanding request Jan 7, 2022

igorbernstein2 marked this pull request as ready for review January 7, 2022 17:36

igorbernstein2 requested review from a team as code owners January 7, 2022 17:36

Merge branch 'main' into dynamic-channel-pool

71f102e

chanseokoh reviewed Jan 11, 2022

View reviewed changes

igorbernstein2 added 2 commits February 11, 2022 15:44

address feedback

3381e9e

Merge remote-tracking branch 'origin/main' into dynamic-channel-pool

774d4ca

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

remove unused import

71d177b

chanseokoh approved these changes Feb 15, 2022

View reviewed changes

inline old factory methods

d82c6b8

chanseokoh merged commit fff2bab into googleapis:main Feb 15, 2022

release-please bot mentioned this pull request Feb 15, 2022

chore(main): release 2.13.0 #1628

Merged

igorbernstein2 deleted the dynamic-channel-pool branch February 15, 2022 16:49

daniel-sanche mentioned this pull request May 24, 2023

v3: dynamic channel pools googleapis/python-bigtable#785

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dynamic channel pool scaled by number of outstanding request #1569

feat: dynamic channel pool scaled by number of outstanding request #1569

igorbernstein2 commented Nov 22, 2021 •

edited

Loading

chanseokoh commented Nov 23, 2021

igorbernstein2 commented Jan 7, 2022

chanseokoh left a comment

chanseokoh Jan 11, 2022

igorbernstein2 Feb 11, 2022

chanseokoh Jan 11, 2022

igorbernstein2 Feb 11, 2022

igorbernstein2 commented Feb 11, 2022

chanseokoh commented Feb 11, 2022

igorbernstein2 commented Feb 14, 2022 •

edited

Loading

chanseokoh left a comment

igorbernstein2 commented Feb 15, 2022

sonarqubecloud bot commented Feb 15, 2022

meltsufin commented Feb 15, 2022

feat: dynamic channel pool scaled by number of outstanding request #1569

feat: dynamic channel pool scaled by number of outstanding request #1569

Conversation

igorbernstein2 commented Nov 22, 2021 • edited Loading

chanseokoh commented Nov 23, 2021

igorbernstein2 commented Jan 7, 2022

chanseokoh left a comment

Choose a reason for hiding this comment

chanseokoh Jan 11, 2022

Choose a reason for hiding this comment

igorbernstein2 Feb 11, 2022

Choose a reason for hiding this comment

chanseokoh Jan 11, 2022

Choose a reason for hiding this comment

igorbernstein2 Feb 11, 2022

Choose a reason for hiding this comment

igorbernstein2 commented Feb 11, 2022

chanseokoh commented Feb 11, 2022

igorbernstein2 commented Feb 14, 2022 • edited Loading

chanseokoh left a comment

Choose a reason for hiding this comment

igorbernstein2 commented Feb 15, 2022

sonarqubecloud bot commented Feb 15, 2022

meltsufin commented Feb 15, 2022

igorbernstein2 commented Nov 22, 2021 •

edited

Loading

igorbernstein2 commented Feb 14, 2022 •

edited

Loading