Conversation
There was a problem hiding this comment.
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.
| 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); | ||
| } |
There was a problem hiding this comment.
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);
}a6d05d8 to
76db06c
Compare
76db06c to
c37bae9
Compare
c37bae9 to
36f49b5
Compare
Makes CloudPath fallback grpc-gcp pool start cold:
DirectPath pool remains eager/fixed.
Impact when DirectPath healthy:
CloudPath still grows on demand if fallback mode activates.
Internal reference: go/grpc-gcp-directpath-fixes