feat(bigtable): refactor channel_pool_factory as we need to maintain two channel factory one for session and another for unary#14652
Conversation
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?
There was a problem hiding this comment.
Done in 71d7f65 — renamed directpathEnvVar to directAccessEnvVar (Go identifier only; env var value stays CBT_ENABLE_DIRECTPATH for back-compat).
| 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
There was a problem hiding this comment.
Done in 71d7f65 — added a doc block on CreateAndStartManagedChannelPool explaining that o is the base option set and directAccessOptions is the conditional, per-connection layer applied only when direct access is enabled.
| 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?
There was a problem hiding this comment.
Done in 71d7f65 — directPathOptions is now directAccessOptions in both client.go and channel_pool_factory.go. (Upstream gax names like internaloption.EnableDirectPath stay as-is.)
|
|
||
| // 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?
There was a problem hiding this comment.
Added a comment in 71d7f65 explaining the validation is a guardrail — ValidateDynamicConfig is the single source of truth for what "valid" means, so any future default tweak or new validation rule is caught at client construction rather than at runtime. Happy to revisit the rename in a follow-up if you'd prefer the config become user-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
Address review nits from googleapis#14652: rename `directpathEnvVar` to `directAccessEnvVar` and `directPathOptions` to `directAccessOptions` so identifiers are consistent with `DisableDirectAccess`, `isDirectAccessEnabled`, and `directAccessMD`. The env var value stays `CBT_ENABLE_DIRECTPATH` — only Go identifiers were touched. Also document why `o` and `directAccessOptions` are passed separately to the channel-pool factories, and why we still validate the "default" DynamicChannelPoolConfig at client construction.
🤖 I have created a release *beep* *boop* --- ## [1.50.0](bigtable/v1.49.0...bigtable/v1.50.0) (2026-06-17) ### Features * **bigtable:** Enable JWT ([#19957](#19957)) ([af93528](af93528)) * **bigtable:** Generate Admin API client using selective gapic ([#19962](#19962)) ([a37509d](a37509d)) * **bigtable:** Refactor channel_pool_factory as we need to maintain two channel factory one for session and another for unary ([#14652](#14652)) ([0c0b6ce](0c0b6ce)) * **bigtable:** Wrap admin client ([#14534](#14534)) ([0bea4ba](0bea4ba)) * Update API sources and regenerate ([#19950](#19950)) ([c7607be](c7607be)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
No description provided.