-
Notifications
You must be signed in to change notification settings - Fork 135
feat: add support of dynamic channel pooling #4265
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for dynamic channel pooling in the Spanner client, which is a significant enhancement. The changes are well-structured across configuration (SpannerOptions), RPC implementation (GapicSpannerRpc), and interceptors (RequestIdInterceptor). The feature is controlled by a comprehensive set of new options, with sensible defaults and validation. The inclusion of extensive unit tests for both the new options and the interceptor logic is commendable. My review includes a couple of suggestions to improve robustness and align the implementation with the documented behavior.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
da0405a to
33a2af2
Compare
78c37c7 to
723f13b
Compare
723f13b to
ce43d4a
Compare
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
Outdated
Show resolved
Hide resolved
|
|
||
| // When dynamic channel pool is enabled, use the DCP initial size as the pool size. | ||
| // When disabled, use the explicitly configured numChannels. | ||
| final int poolSize = options.isDynamicChannelPoolEnabled() ? 0 : options.getNumChannels(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change the behavior for customers who are currently using grpc-gcp? (I would think not, considering that it seems like the channel pool size was set to options.getNumChannels() before this change as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because when poolSize = 0, grpc-gcp uses initSize from GcpChannelPoolOptions. Previously, numChannels was used as the pool size. The change is intentional to let DCP control initialization.
Description
This PR adds support for Dynamic Channel Pooling (DCP), which enables automatic scaling of gRPC channels based on load.
Dynamic Channel Pool is disabled by default and must be explicitly enabled via
enableDynamicChannelPool(). DCP will be enabled by default in a future release.Basic Usage (with Spanner defaults)
Custom Configuration
For custom configuration, use
setGcpChannelPoolOptions(GcpChannelPoolOptions):Spanner Default Configuration
When
enableDynamicChannelPool()is called without customGcpChannelPoolOptions, Spanner-specific defaults are applied viaSpannerOptions.createDefaultDynamicChannelPoolOptions():API Summary
enableDynamicChannelPool()disableDynamicChannelPool()isDynamicChannelPoolEnabled()setGcpChannelPoolOptions(GcpChannelPoolOptions)getGcpChannelPoolOptions()SpannerOptions.createDefaultDynamicChannelPoolOptions()Behavior Notes
setNumChannels(int)disables DCP even if explicitly enabledX-Goog-Spanner-Request-Id) now reflect the actual physical channel selected by grpc-gcp, rather than the logical channel hint computed by the clientGcpChannelPoolOptions.Builder