chore(spanner): Auto-enable location API and dynamic channel pool for experimental hosts#12679
chore(spanner): Auto-enable location API and dynamic channel pool for experimental hosts#12679
Conversation
… experimental hosts, and merge default dynamic channel pool options with user-provided options When an experimental host is configured, automatically enable the location API (unless explicitly disabled via GOOGLE_SPANNER_EXPERIMENTAL_LOCATION_API=false) and dynamic channel pool (unless explicitly disabled via disableDynamicChannelPool()). Also fix an issue where setting custom GcpChannelPoolOptions without specifying dynamic scaling parameters (minRpcPerChannel, maxRpcPerChannel, scaleDownInterval) would reset them to zero, preventing channel scale-up. Custom options are now merged with Spanner defaults so unset fields retain their default values.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the default configuration for the Dynamic Channel Pool (DCP) in SpannerOptions, increasing the maximum RPCs per channel and maximum channels while decreasing initial and minimum channel counts. It introduces a merging mechanism for user-provided pool options and automatically enables DCP and the Location API when an experimental host is configured. Review feedback points out a logic error in environment variable parsing for the Location API and a potential invariant violation in the pool options merging logic that could lead to runtime exceptions.
java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
| if (userOptions.getMinRpcPerChannel() <= 0 | ||
| || userOptions.getMaxRpcPerChannel() <= 0 | ||
| || userOptions.getScaleDownInterval() == null | ||
| || userOptions.getScaleDownInterval().isZero()) { | ||
| merged.setDynamicScaling( | ||
| userOptions.getMinRpcPerChannel() > 0 | ||
| ? userOptions.getMinRpcPerChannel() | ||
| : defaults.getMinRpcPerChannel(), | ||
| userOptions.getMaxRpcPerChannel() > 0 | ||
| ? userOptions.getMaxRpcPerChannel() | ||
| : defaults.getMaxRpcPerChannel(), | ||
| userOptions.getScaleDownInterval() != null && !userOptions.getScaleDownInterval().isZero() | ||
| ? userOptions.getScaleDownInterval() | ||
| : defaults.getScaleDownInterval()); | ||
| } |
There was a problem hiding this comment.
The merging logic for dynamic scaling parameters can lead to an invalid state where minRpcPerChannel is greater than maxRpcPerChannel, which will cause an IllegalArgumentException when the options are built. For example, if a user provides a custom maxRpcPerChannel of 10 but leaves minRpcPerChannel unset (0), the merged result will have minRpcPerChannel=15 (the default) and maxRpcPerChannel=10.
Consider ensuring that the merged values maintain the invariant minRpcPerChannel <= maxRpcPerChannel by capping the minimum or bumping the maximum during the merge.
1 participant
Auto-enable location API for experimental hosts: When
setExperimentalHost()is used,enableLocationApinow defaults totrue. Users can still explicitly opt out by settingGOOGLE_SPANNER_EXPERIMENTAL_LOCATION_API=false.Auto-enable dynamic channel pool for experimental hosts: When
setExperimentalHost()isused and the user has not explicitly called
enableDynamicChannelPool()ordisableDynamicChannelPool(), dynamic channel pooling is enabled by default(subject to existing grpc-gcp and numChannels constraints).
Merge custom GcpChannelPoolOptions with Spanner defaults: Previously, providing custom
GcpChannelPoolOptions(e.g., only settingmaxSize,minSize,initSize) would leavedynamic scaling parameters (
minRpcPerChannel,maxRpcPerChannel,scaleDownInterval) atzero, silently breaking channel scale-up. Now, any unset fields in user-provided options are
filled in from Spanner's defaults via
mergeWithDefaultChannelPoolOptions().