-
Notifications
You must be signed in to change notification settings - Fork 136
feat: Add Dynamic Channel Pooling (DCP) support to Connection API #4299
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
Summary of ChangesHello @rahul2393, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces support for Dynamic Channel Pooling (DCP) within the Connection API, providing a mechanism for the Spanner client to automatically adjust the number of gRPC channels based on real-time load. This feature aims to optimize resource utilization and improve performance by ensuring that the client can scale its channel capacity up or down as needed, without manual intervention. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 adds support for Dynamic Channel Pooling (DCP) to the Spanner Connection API. The changes are well-implemented and include new connection properties, logic to configure the Spanner client with DCP settings, and corresponding tests. I have one suggestion to improve the maintainability of the code that configures the DCP options.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerPool.java
Outdated
Show resolved
Hide resolved
| ENABLE_DYNAMIC_CHANNEL_POOL_PROPERTY_NAME, | ||
| "Enable dynamic channel pooling for automatic gRPC channel scaling. When enabled, the " | ||
| + "client will automatically scale the number of channels based on load. Setting " | ||
| + "numChannels will disable dynamic channel pooling even if this is set to true.", |
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.
nit: Would you mind adding something like `The default is SpannerOptions.DEFAULT_ENABLE_DYNAMIC_CHANNEL_POOL (false). This default will be changed to true in a future version.'? (At least, assuming that this is correct)
| } | ||
| // Configure Dynamic Channel Pooling (DCP) if enabled. | ||
| // Note: Setting numChannels disables DCP even if enableDynamicChannelPool is true. | ||
| if (Boolean.TRUE.equals(key.enableDynamicChannelPool) && key.numChannels == null) { |
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.
This assumes that dynamic channel pooling is disabled by default. This is currently correct, but is likely to change in the future. Can we therefore either:
- Add explicit behavior here to disable DCP if
enableDynamicChannelPoolhas been set to false? - Or add a test to the Connection API that will fail if the default setting for DCP in the client library changes?
What I mean is the following:
- A user that sets
enableDynamicChannelPool=trueshould always get DCP. This works fine now. - A user that does not set a value for
enableDynamicChannelPoolshould get the default of the client library. That works fine now, and the default is that it is disabled. - A user that sets
enableDynamicChannelPool=falseshould always get a fixed channel pool. That works now, but if the default in the client library changes, then they would all of a sudden get DCP.
| } | ||
|
|
||
| @Test | ||
| public void testEnableDynamicChannelPool() { |
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.
There should also be a test for testDisableDynamicChannelPool
Summary
Details
This PR exposes the Dynamic Channel Pooling feature (already available in SpannerOptions)
through the Connection API and JDBC driver via connection URL properties.
New Connection Properties
enableDynamicChannelPooldcpMinChannelsdcpMaxChannelsdcpInitialChannelsExample Usage
jdbc:cloudspanner:/projects/my-project/instances/my-instance/databases/my-db?enableDynamicChannelPool=true;dcpMinChannels=3;dcpMaxChannels=15
Important Notes
numChannelsdisables DCP even ifenableDynamicChannelPool=true