Skip to content

fix(spanner): lazy cloudpath fallback#13156

Open
rahul2393 wants to merge 3 commits into
mainfrom
directpath-fix-2-lazy-cloudpath-fallback
Open

fix(spanner): lazy cloudpath fallback#13156
rahul2393 wants to merge 3 commits into
mainfrom
directpath-fix-2-lazy-cloudpath-fallback

Conversation

@rahul2393
Copy link
Copy Markdown
Contributor

@rahul2393 rahul2393 commented May 11, 2026

Makes CloudPath fallback grpc-gcp pool start cold:

  maxSize = numChannels
  minSize = 0
  initSize = 0

DirectPath pool remains eager/fixed.

Impact when DirectPath healthy:

  48 ChannelRef -> 24 ChannelRef

CloudPath still grows on demand if fallback mode activates.
Internal reference: go/grpc-gcp-directpath-fixes

@rahul2393 rahul2393 requested review from a team as code owners May 11, 2026 04:23
@rahul2393 rahul2393 changed the title fix(spanner): directpath fix 2 lazy cloudpath fallback fix(spanner): lazy cloudpath fallback May 11, 2026
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 implement lazy initialization for the cloud path during DirectPath fallback when the gRPC GCP extension is enabled. It updates FallbackChannelBuilder to accept generic ManagedChannelBuilder instances and introduces specialized channel pool options for lazy fallback. Additionally, new unit tests use reflection to validate the creation of GCP managed channels. Review feedback highlights a regression where missing null checks on GCP options could lead to NullPointerException and points out redundant logic that should be refactored for better safety and performance.

Comment on lines +704 to 719
ManagedChannelBuilder<?> primaryBuilder = builder;
ManagedChannelBuilder<?> fallbackBuilder = cloudPathBuilder;
if (options.isGrpcGcpExtensionEnabled()) {
String jsonApiConfig = parseGrpcGcpApiConfig();
GcpManagedChannelOptions primaryGcpOptions = grpcGcpOptionsWithMetricsAndDcp(options);
GcpManagedChannelOptions fallbackGcpOptions =
grpcGcpOptionsWithMetricsAndLazyFallback(options);
primaryBuilder =
GcpManagedChannelBuilder.forDelegateBuilder(builder)
.withApiConfigJsonString(jsonApiConfig)
.withOptions(primaryGcpOptions);
fallbackBuilder =
GcpManagedChannelBuilder.forDelegateBuilder(cloudPathBuilder)
.withApiConfigJsonString(jsonApiConfig)
.withOptions(fallbackGcpOptions);
}
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 logic for configuring GCP Managed Channel options has a regression compared to the previous implementation. Specifically, the null check for grpcGcpOptionsWithMetricsAndDcp(options) has been removed, which can lead to a NullPointerException when calling GcpManagedChannelOptions.newBuilder(...). Additionally, grpcGcpOptionsWithMetricsAndDcp(options) is called twice (once directly and once inside grpcGcpOptionsWithMetricsAndLazyFallback), which is redundant.

I recommend refactoring this block to compute the base options once, ensure they are non-null, and then derive the fallback options from them.

          ManagedChannelBuilder<?> primaryBuilder = builder;
          ManagedChannelBuilder<?> fallbackBuilder = cloudPathBuilder;
          if (options.isGrpcGcpExtensionEnabled()) {
            String jsonApiConfig = parseGrpcGcpApiConfig();
            GcpManagedChannelOptions primaryGcpOptions = grpcGcpOptionsWithMetricsAndDcp(options);
            if (primaryGcpOptions == null) {
              primaryGcpOptions = GcpManagedChannelOptions.newBuilder().build();
            }
            GcpManagedChannelOptions fallbackGcpOptions =
                GcpManagedChannelOptions.newBuilder(primaryGcpOptions)
                    .withChannelPoolOptions(getGrpcGcpLazyFallbackChannelPoolOptions(options))
                    .build();
            primaryBuilder =
                GcpManagedChannelBuilder.forDelegateBuilder(builder)
                    .withApiConfigJsonString(jsonApiConfig)
                    .withOptions(primaryGcpOptions);
            fallbackBuilder =
                GcpManagedChannelBuilder.forDelegateBuilder(cloudPathBuilder)
                    .withApiConfigJsonString(jsonApiConfig)
                    .withOptions(fallbackGcpOptions);
          }

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.

Done

@rahul2393 rahul2393 force-pushed the directpath-fix-2-lazy-cloudpath-fallback branch 2 times, most recently from a6d05d8 to 76db06c Compare May 11, 2026 05:01
@rahul2393 rahul2393 force-pushed the directpath-fix-2-lazy-cloudpath-fallback branch from 76db06c to c37bae9 Compare May 11, 2026 05:11
@rahul2393 rahul2393 force-pushed the directpath-fix-2-lazy-cloudpath-fallback branch from c37bae9 to 36f49b5 Compare May 11, 2026 05:12
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