fix(spanner): avoid double grpc-gcp wrapping for directpath fallback#13155
fix(spanner): avoid double grpc-gcp wrapping for directpath fallback#13155
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors GapicSpannerRpc to conditionally apply the gRPC GCP extension and updates FallbackChannelBuilder to use generic ManagedChannelBuilder types. It also adds tests that use reflection to monitor gRPC channel creation. The review feedback highlights a missing method definition for createBaseChannelProviderBuilder and potential inconsistencies in its application. Additionally, a correction was suggested for the test configuration to ensure that GCP fallback is explicitly enabled, preventing tests from being skipped.
| final CredentialsProvider credentialsProvider) { | ||
| InstantiatingGrpcChannelProvider.Builder cloudPathProviderBuilder = | ||
| createChannelProviderBuilder( | ||
| createBaseChannelProviderBuilder( |
There was a problem hiding this comment.
The method createBaseChannelProviderBuilder is called here, but its definition is not included in the pull request. Additionally, the call site at line 371 (visible in context but not in the diff) still uses createChannelProviderBuilder. If createBaseChannelProviderBuilder is a new method intended to provide a base configuration without the GCP extension, please ensure its definition is included and consider if other call sites should also be updated for consistency.
There was a problem hiding this comment.
It's there in line 746 createBaseChannelProviderBuilder
| .setEnableDirectAccess(true) | ||
| .setHost("http://localhost:1") |
There was a problem hiding this comment.
The SpannerOptions builder in createDirectPathFallbackObjectCountOptions does not explicitly enable GCP fallback. Since isEnableGcpFallback() defaults to false in SpannerOptions, the assumeTrue checks in the tests (e.g., at line 1442 and 1465) will likely cause these tests to be skipped. Explicitly enabling it here ensures the tests actually execute the DirectPath fallback logic.
| .setEnableDirectAccess(true) | |
| .setHost("http://localhost:1") | |
| .setEnableDirectAccess(true) | |
| .setEnableGcpFallback(true) | |
| .setHost("http://localhost:1") |
41d94da to
b02389f
Compare
Fixes DirectPath fallback channel construction so each path gets only one grpc-gcp layer.
Before:
After:
Impact:
Internal reference: go/grpc-gcp-directpath-fixes