Skip to content

fix(spanner): avoid double grpc-gcp wrapping for directpath fallback#13155

Open
rahul2393 wants to merge 2 commits intomainfrom
directpath-fix-1-remove-double-grpc-gcp-wrap
Open

fix(spanner): avoid double grpc-gcp wrapping for directpath fallback#13155
rahul2393 wants to merge 2 commits intomainfrom
directpath-fix-1-remove-double-grpc-gcp-wrap

Conversation

@rahul2393
Copy link
Copy Markdown
Contributor

@rahul2393 rahul2393 commented May 11, 2026

Fixes DirectPath fallback channel construction so each path gets only one grpc-gcp layer.

Before:

  GcpManagedChannel -> ChannelRef -> inner GcpManagedChannel -> ChannelRef

After:

  GcpFallbackChannel
    -> DirectPath GcpManagedChannel
    -> CloudPath GcpManagedChannel

Impact:

  432 ChannelRef -> 48 ChannelRef
  54 GcpManagedChannel -> 6 GcpManagedChannel

Internal reference: go/grpc-gcp-directpath-fixes

@rahul2393 rahul2393 requested review from a team as code owners May 11, 2026 04:22
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 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(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's there in line 746 createBaseChannelProviderBuilder

Comment on lines +1484 to +1485
.setEnableDirectAccess(true)
.setHost("http://localhost:1")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
.setEnableDirectAccess(true)
.setHost("http://localhost:1")
.setEnableDirectAccess(true)
.setEnableGcpFallback(true)
.setHost("http://localhost:1")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default is true

@rahul2393 rahul2393 force-pushed the directpath-fix-1-remove-double-grpc-gcp-wrap branch from 41d94da to b02389f Compare May 11, 2026 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant