feat(bigtable): refactor channel_pool_factory as we need to maintain two channel factory one for session and another for unary#14652
Conversation
product-auto-label
Bot
added
the
api: bigtable
![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 refactors the Bigtable client initialization by extracting the connection pool creation and lifecycle management into a new managedChannelPool struct and helper functions in channel_pool_factory.go. Feedback on the changes highlights a regression where the direct access dialer is unconditionally configured instead of respecting the allowDirectAccess configuration, and points out a leftover debug print statement that should be removed.
| ) | ||
|
|
||
| const ( | ||
| directpathEnvVar = "CBT_ENABLE_DIRECTPATH" |
There was a problem hiding this comment.
nit maybe: directpath -> directAccess so it's consistent?
| project, instance string, | ||
| config ChannelPoolConfig, | ||
| otelMeterProvider metric.MeterProvider, | ||
| o []option.ClientOption, |
There was a problem hiding this comment.
maybe add a comment explaining why we need 2 ClientOptions
| config ChannelPoolConfig, | ||
| otelMeterProvider metric.MeterProvider, | ||
| o []option.ClientOption, | ||
| directPathOptions []option.ClientOption, |
There was a problem hiding this comment.
also should we rename all directpath to directAccess?
|
|
||
| // Validate dynamic config early if enabled | ||
| if !config.DisableDynamicChannelPool { | ||
| if err := ValidateDynamicConfig(btopt.DefaultDynamicChannelPoolConfig(), defaultBigtableConnPoolSize); err != nil { |
There was a problem hiding this comment.
nit: this method is called "DefaultDynamicChannelPoolConfig". If it's default, can it still be invalid?
Or maybe rename it to DynamicChannelPoolConfig if we want it to be configurable?
| WithFeatureFlagsMetadata(directAccessMD), | ||
| WithMetricsReporterConfig(btopt.DefaultMetricsReporterConfig()), | ||
| WithMeterProvider(otelMeterProvider), | ||
| WithDirectAccessFeatureFlagsMetadata(directAccessMD), |
There was a problem hiding this comment.
I realized this part was copied from the client.go, but maybe reordering and add a comment to explain the duplicate?
...
WithFeatureFlagsMetadata(directAccessMD),
WithDirectAccessFeatureFlagsMetadata(directAccessMD),
...
There was a problem hiding this comment.
After some digging I am still not convinced of the necessity for two separate feature flag fields in the BigtableChannelPool... Looking at server side implementation, traffic_director_enabled appears to be purely informational, used primarily for populating request metrics and observability data. While direct_access_requested does influence routing decisions, the server-side check for this flag is already nested within logic that only evaluates traffic originating from Traffic Director?
Consequently, there is no functional use case for 'flipping' these flags based on the transport type (Cloud Path vs. Direct Path). And I realized that the Java client is also inconsistent: the newer session-based client explicitly downgrades these flags to false upon Direct Path failure (https://github.com/googleapis/google-cloud-java/blob/ff45dd2400e65d06d1bd3e3cf7fb1cd1cb9c3074/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ShimImpl.java#L128), whereas the classic client treats them as immutable settings once the client is initialized (https://github.com/googleapis/google-cloud-java/blob/ff45dd2400e65d06d1bd3e3cf7fb1cd1cb9c3074/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java#L660-L661).
Since these flags represent the intrinsic capabilities and opt-in state of the client rather than the ephemeral state of a specific connection, they should remain immutable for the lifetime of the client settings in my opinion?
But maybe I missed some use case and maybe need to ask Igor to clarify the intent of these 2 feature flags.. I'll check on Monday.
And even if we do need refactor, we can do it in a separate PR so feel free to merge after addressing other nits.
There was a problem hiding this comment.
Ok we can clean this up in a separate PR
3 participants
No description provided.