-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(spanner): remove directpath path and only use cloudpath admin clients #13157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -372,16 +372,18 @@ public GapicSpannerRpc(final SpannerOptions options) { | |||||||||||||||||||||||||||||||||||||||
| options, headerProviderWithUserAgent, isEnableDirectAccess); | ||||||||||||||||||||||||||||||||||||||||
| GrpcGcpEndpointChannelConfigurator endpointChannelConfigurator = | ||||||||||||||||||||||||||||||||||||||||
| createGrpcGcpEndpointChannelConfigurator(defaultChannelProviderBuilder, options); | ||||||||||||||||||||||||||||||||||||||||
| maybeEnableGrpcGcpExtension(defaultChannelProviderBuilder, options); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (options.getChannelProvider() == null | ||||||||||||||||||||||||||||||||||||||||
| && isEnableDirectAccess | ||||||||||||||||||||||||||||||||||||||||
| && options.isEnableGcpFallback()) { | ||||||||||||||||||||||||||||||||||||||||
| boolean useGcpFallback = | ||||||||||||||||||||||||||||||||||||||||
| options.getChannelProvider() == null | ||||||||||||||||||||||||||||||||||||||||
| && isEnableDirectAccess | ||||||||||||||||||||||||||||||||||||||||
| && options.isEnableGcpFallback(); | ||||||||||||||||||||||||||||||||||||||||
| if (useGcpFallback) { | ||||||||||||||||||||||||||||||||||||||||
| setupGcpFallback( | ||||||||||||||||||||||||||||||||||||||||
| defaultChannelProviderBuilder, | ||||||||||||||||||||||||||||||||||||||||
| options, | ||||||||||||||||||||||||||||||||||||||||
| headerProviderWithUserAgent, | ||||||||||||||||||||||||||||||||||||||||
| credentialsProvider); | ||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||
| maybeEnableGrpcGcpExtension(defaultChannelProviderBuilder, options); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| boolean enableLocationApi = options.isEnableLocationApi(); | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -397,6 +399,12 @@ public GapicSpannerRpc(final SpannerOptions options) { | |||||||||||||||||||||||||||||||||||||||
| options.getChannelEndpointCacheFactory(), | ||||||||||||||||||||||||||||||||||||||||
| endpointChannelConfigurator) | ||||||||||||||||||||||||||||||||||||||||
| : baseChannelProvider; | ||||||||||||||||||||||||||||||||||||||||
| TransportChannelProvider adminChannelProvider = | ||||||||||||||||||||||||||||||||||||||||
| useGcpFallback | ||||||||||||||||||||||||||||||||||||||||
| ? createChannelProviderBuilder( | ||||||||||||||||||||||||||||||||||||||||
| options, headerProviderWithUserAgent, /* isEnableDirectAccess= */ false) | ||||||||||||||||||||||||||||||||||||||||
| .build() | ||||||||||||||||||||||||||||||||||||||||
| : channelProvider; | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+402
to
+407
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| spannerWatchdog = | ||||||||||||||||||||||||||||||||||||||||
| Executors.newSingleThreadScheduledExecutor( | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -495,7 +503,7 @@ public GapicSpannerRpc(final SpannerOptions options) { | |||||||||||||||||||||||||||||||||||||||
| pdmlSettings.build(), clientContext); | ||||||||||||||||||||||||||||||||||||||||
| this.instanceAdminStubSettings = | ||||||||||||||||||||||||||||||||||||||||
| options.getInstanceAdminStubSettings().toBuilder() | ||||||||||||||||||||||||||||||||||||||||
| .setTransportChannelProvider(channelProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setTransportChannelProvider(adminChannelProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setCredentialsProvider(credentialsProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setStreamWatchdogProvider(watchdogProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setTracerFactory( | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -506,7 +514,7 @@ public GapicSpannerRpc(final SpannerOptions options) { | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| this.databaseAdminStubSettings = | ||||||||||||||||||||||||||||||||||||||||
| options.getDatabaseAdminStubSettings().toBuilder() | ||||||||||||||||||||||||||||||||||||||||
| .setTransportChannelProvider(channelProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setTransportChannelProvider(adminChannelProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setCredentialsProvider(credentialsProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setStreamWatchdogProvider(watchdogProvider) | ||||||||||||||||||||||||||||||||||||||||
| .setTracerFactory( | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -656,8 +664,11 @@ private void setupGcpFallback( | |||||||||||||||||||||||||||||||||||||||
| final HeaderProvider headerProviderWithUserAgent, | ||||||||||||||||||||||||||||||||||||||||
| final CredentialsProvider credentialsProvider) { | ||||||||||||||||||||||||||||||||||||||||
| InstantiatingGrpcChannelProvider.Builder cloudPathProviderBuilder = | ||||||||||||||||||||||||||||||||||||||||
| createChannelProviderBuilder( | ||||||||||||||||||||||||||||||||||||||||
| createBaseChannelProviderBuilder( | ||||||||||||||||||||||||||||||||||||||||
| options, headerProviderWithUserAgent, /* isEnableDirectAccess= */ false); | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
666
to
668
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| if (options.isGrpcGcpExtensionEnabled()) { | ||||||||||||||||||||||||||||||||||||||||
| cloudPathProviderBuilder.setPoolSize(1); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| InstantiatingGrpcChannelProvider cloudPathProvider = cloudPathProviderBuilder.build(); | ||||||||||||||||||||||||||||||||||||||||
| ManagedChannelBuilder cloudPathBuilder; | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -689,29 +700,34 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall( | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> existingConfigurator = | ||||||||||||||||||||||||||||||||||||||||
| defaultChannelProviderBuilder.getChannelConfigurator(); | ||||||||||||||||||||||||||||||||||||||||
| if (options.isGrpcGcpExtensionEnabled()) { | ||||||||||||||||||||||||||||||||||||||||
| defaultChannelProviderBuilder.setPoolSize(1); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| defaultChannelProviderBuilder.setChannelConfigurator( | ||||||||||||||||||||||||||||||||||||||||
| directPathBuilder -> { | ||||||||||||||||||||||||||||||||||||||||
| ManagedChannelBuilder builder = directPathBuilder; | ||||||||||||||||||||||||||||||||||||||||
| if (existingConfigurator != null) { | ||||||||||||||||||||||||||||||||||||||||
| builder = existingConfigurator.apply(builder); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| String jsonApiConfig = parseGrpcGcpApiConfig(); | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelOptions gcpOptions = grpcGcpOptionsWithMetricsAndDcp(options); | ||||||||||||||||||||||||||||||||||||||||
| if (gcpOptions == null) { | ||||||||||||||||||||||||||||||||||||||||
| gcpOptions = GcpManagedChannelOptions.newBuilder().build(); | ||||||||||||||||||||||||||||||||||||||||
| ManagedChannelBuilder<?> primaryBuilder = builder; | ||||||||||||||||||||||||||||||||||||||||
| ManagedChannelBuilder<?> fallbackBuilder = cloudPathBuilder; | ||||||||||||||||||||||||||||||||||||||||
| if (options.isGrpcGcpExtensionEnabled()) { | ||||||||||||||||||||||||||||||||||||||||
| String jsonApiConfig = parseGrpcGcpApiConfig(); | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelOptions gcpOptions = grpcGcpOptionsWithMetricsAndDcp(options); | ||||||||||||||||||||||||||||||||||||||||
| if (gcpOptions == null) { | ||||||||||||||||||||||||||||||||||||||||
| gcpOptions = GcpManagedChannelOptions.newBuilder().build(); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| primaryBuilder = | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder.forDelegateBuilder(builder) | ||||||||||||||||||||||||||||||||||||||||
| .withApiConfigJsonString(jsonApiConfig) | ||||||||||||||||||||||||||||||||||||||||
| .withOptions(gcpOptions); | ||||||||||||||||||||||||||||||||||||||||
| fallbackBuilder = | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder.forDelegateBuilder(cloudPathBuilder) | ||||||||||||||||||||||||||||||||||||||||
| .withApiConfigJsonString(jsonApiConfig) | ||||||||||||||||||||||||||||||||||||||||
| .withOptions(gcpOptions); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder primaryGcpBuilder = | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder.forDelegateBuilder(builder) | ||||||||||||||||||||||||||||||||||||||||
| .withApiConfigJsonString(jsonApiConfig) | ||||||||||||||||||||||||||||||||||||||||
| .withOptions(gcpOptions); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder fallbackGcpBuilder = | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder.forDelegateBuilder(cloudPathBuilder) | ||||||||||||||||||||||||||||||||||||||||
| .withApiConfigJsonString(jsonApiConfig) | ||||||||||||||||||||||||||||||||||||||||
| .withOptions(gcpOptions); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| GcpFallbackOpenTelemetry fallbackTelemetry = | ||||||||||||||||||||||||||||||||||||||||
| GcpFallbackOpenTelemetry.newBuilder() | ||||||||||||||||||||||||||||||||||||||||
| .withSdk(getFallbackOpenTelemetry(options)) | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -720,9 +736,7 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall( | |||||||||||||||||||||||||||||||||||||||
| .build(); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return new FallbackChannelBuilder( | ||||||||||||||||||||||||||||||||||||||||
| primaryGcpBuilder, | ||||||||||||||||||||||||||||||||||||||||
| fallbackGcpBuilder, | ||||||||||||||||||||||||||||||||||||||||
| createFallbackChannelOptions(fallbackTelemetry, 1)); | ||||||||||||||||||||||||||||||||||||||||
| primaryBuilder, fallbackBuilder, createFallbackChannelOptions(fallbackTelemetry, 1)); | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2595,15 +2609,15 @@ private static class FallbackChannelBuilder | |||||||||||||||||||||||||||||||||||||||
| extends ForwardingChannelBuilder2<FallbackChannelBuilder> { | ||||||||||||||||||||||||||||||||||||||||
| private final GcpFallbackChannelOptions options; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private final GcpManagedChannelBuilder primaryGcpBuilder; | ||||||||||||||||||||||||||||||||||||||||
| private final GcpManagedChannelBuilder fallbackGcpBuilder; | ||||||||||||||||||||||||||||||||||||||||
| private final ManagedChannelBuilder<?> primaryBuilder; | ||||||||||||||||||||||||||||||||||||||||
| private final ManagedChannelBuilder<?> fallbackBuilder; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private FallbackChannelBuilder( | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder primary, | ||||||||||||||||||||||||||||||||||||||||
| GcpManagedChannelBuilder fallback, | ||||||||||||||||||||||||||||||||||||||||
| ManagedChannelBuilder<?> primary, | ||||||||||||||||||||||||||||||||||||||||
| ManagedChannelBuilder<?> fallback, | ||||||||||||||||||||||||||||||||||||||||
| GcpFallbackChannelOptions options) { | ||||||||||||||||||||||||||||||||||||||||
| this.primaryGcpBuilder = primary; | ||||||||||||||||||||||||||||||||||||||||
| this.fallbackGcpBuilder = fallback; | ||||||||||||||||||||||||||||||||||||||||
| this.primaryBuilder = primary; | ||||||||||||||||||||||||||||||||||||||||
| this.fallbackBuilder = fallback; | ||||||||||||||||||||||||||||||||||||||||
| this.options = options; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2613,7 +2627,7 @@ private FallbackChannelBuilder( | |||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||
| protected ManagedChannelBuilder<?> delegate() { | ||||||||||||||||||||||||||||||||||||||||
| return primaryGcpBuilder; | ||||||||||||||||||||||||||||||||||||||||
| return primaryBuilder; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2622,7 +2636,7 @@ protected ManagedChannelBuilder<?> delegate() { | |||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||
| public ManagedChannel build() { | ||||||||||||||||||||||||||||||||||||||||
| return new GcpFallbackChannel(options, primaryGcpBuilder, fallbackGcpBuilder); | ||||||||||||||||||||||||||||||||||||||||
| return new GcpFallbackChannel(options, primaryBuilder, fallbackBuilder); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the GCP extension is enabled, the GAX pool size should be set to 1 to avoid redundant connection pooling, as the extension manages its own internal pool. This ensures consistency with the logic used when fallback is enabled (see line 701).