From d116c64ab781075cf9e9f7ec6244b665b7dacbba Mon Sep 17 00:00:00 2001 From: kinsaurralde Date: Tue, 7 Apr 2026 20:40:06 +0000 Subject: [PATCH 1/3] Use new createDecoratedChannelBuilder to simplify EEF creation --- .../cloud/spanner/spi/v1/GapicSpannerRpc.java | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java index 2faa3a62fb99..9ab2fdcc0382 100644 --- a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java +++ b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java @@ -229,7 +229,6 @@ import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -614,27 +613,13 @@ private void setupGcpFallback( createChannelProviderBuilder( options, headerProviderWithUserAgent, /* isEnableDirectAccess= */ false); - final ApiFunction existingCloudPathConfigurator = - cloudPathProviderBuilder.getChannelConfigurator(); - final AtomicReference cloudPathBuilderRef = new AtomicReference<>(); - cloudPathProviderBuilder.setChannelConfigurator( - builder -> { - ManagedChannelBuilder effectiveBuilder = builder; - if (existingCloudPathConfigurator != null) { - effectiveBuilder = existingCloudPathConfigurator.apply(effectiveBuilder); - } - cloudPathBuilderRef.set(effectiveBuilder); - return effectiveBuilder; - }); - - // Build the cloudPathProvider to extract the builder which will be provided to - // FallbackChannelBuilder. - try (TransportChannel ignored = cloudPathProviderBuilder.build().getTransportChannel()) { - } catch (Exception e) { + InstantiatingGrpcChannelProvider cloudPathProvider = cloudPathProviderBuilder.build(); + ManagedChannelBuilder cloudPathBuilder; + try { + cloudPathBuilder = cloudPathProvider.createDecoratedChannelBuilder(); + } catch (IOException e) { throw asSpannerException(e); } - - ManagedChannelBuilder cloudPathBuilder = cloudPathBuilderRef.get(); if (cloudPathBuilder == null) { throw new IllegalStateException("CloudPath builder was not captured."); } From d3b962210d555fc965eb329ac9aa48ac2e19189c Mon Sep 17 00:00:00 2001 From: kinsaurralde Date: Tue, 7 Apr 2026 22:21:45 +0000 Subject: [PATCH 2/3] enable GOOGLE_SPANNER_ENABLE_GCP_FALLBACK by default --- .../google/cloud/spanner/SpannerOptions.java | 5 +- .../cloud/spanner/spi/v1/GapicSpannerRpc.java | 1 + .../spanner/spi/v1/GapicSpannerRpcTest.java | 196 ++++++++---------- 3 files changed, 91 insertions(+), 111 deletions(-) diff --git a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java index fac703dbb09a..ce8ad7ba2614 100644 --- a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java +++ b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java @@ -1041,7 +1041,7 @@ default boolean isEnableDirectAccess() { } default boolean isEnableGcpFallback() { - return false; + return true; } default boolean isEnableBuiltInMetrics() { @@ -1136,7 +1136,8 @@ public boolean isEnableDirectAccess() { @Override public boolean isEnableGcpFallback() { - return Boolean.parseBoolean(System.getenv(GOOGLE_SPANNER_ENABLE_GCP_FALLBACK)); + String enableGcpFallback = System.getenv(GOOGLE_SPANNER_ENABLE_GCP_FALLBACK); + return enableGcpFallback == null ? true : Boolean.parseBoolean(enableGcpFallback); } @Override diff --git a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java index 6466ac8de215..c091fab2426f 100644 --- a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java +++ b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java @@ -563,6 +563,7 @@ GcpFallbackChannelOptions createFallbackChannelOptions( .setPrimaryChannelName("directpath") .setFallbackChannelName("cloudpath") .setMinFailedCalls(minFailedCalls) + .setPeriod(Duration.ofMinutes(3)) .setGcpFallbackOpenTelemetry(fallbackTelemetry) .build(); } diff --git a/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java b/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java index 7a6a9f78d4b5..f74fbdd3272e 100644 --- a/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java +++ b/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java @@ -1164,61 +1164,50 @@ public void testFallbackIntegration_doesNotSwitchWhenThresholdNotMet() throws Ex OpenTelemetrySdk openTelemetry = OpenTelemetrySdk.builder().setMeterProvider(meterProvider).build(); - SpannerOptions.useEnvironment( - new SpannerOptions.SpannerEnvironment() { - @Override - public boolean isEnableGcpFallback() { - return true; - } - }); + SpannerOptions.Builder builder = + SpannerOptions.newBuilder() + .setProjectId("test-project") + .setEnableDirectAccess(true) + .setHost("http://localhost:1") // Closed port + .setCredentials(NoCredentials.getInstance()) + .setOpenTelemetry(openTelemetry); + // Make sure the ExecuteBatchDml RPC fails quickly to keep the test fast. + // Note that the timeout is actually not used. It is the fact that it does not retry that + // makes it fail fast. + builder + .getSpannerStubSettingsBuilder() + .executeBatchDmlSettings() + .setSimpleTimeoutNoRetriesDuration(Duration.ofSeconds(10)); + // Setup Options with invalid host to force error + SpannerOptions options = builder.build(); + + TestableGapicSpannerRpc rpc = new TestableGapicSpannerRpc(options); try { - SpannerOptions.Builder builder = - SpannerOptions.newBuilder() - .setProjectId("test-project") - .setEnableDirectAccess(true) - .setHost("http://localhost:1") // Closed port - .setCredentials(NoCredentials.getInstance()) - .setOpenTelemetry(openTelemetry); - // Make sure the ExecuteBatchDml RPC fails quickly to keep the test fast. - // Note that the timeout is actually not used. It is the fact that it does not retry that - // makes it fail fast. - builder - .getSpannerStubSettingsBuilder() - .executeBatchDmlSettings() - .setSimpleTimeoutNoRetriesDuration(Duration.ofSeconds(10)); - // Setup Options with invalid host to force error - SpannerOptions options = builder.build(); - - TestableGapicSpannerRpc rpc = new TestableGapicSpannerRpc(options); - try { - // Make a call that is expected to fail - SpannerException exception = - assertThrows( - SpannerException.class, - () -> - rpc.executeBatchDml( - com.google.spanner.v1.ExecuteBatchDmlRequest.newBuilder() - .setSession("projects/p/instances/i/databases/d/sessions/s") - .build(), - null)); - assertEquals(ErrorCode.UNAVAILABLE, exception.getErrorCode()); - - // Wait briefly for the 10ms period to trigger the fallback check - Thread.sleep(10); - - // Verify Fallback via Metrics - Collection metrics = metricReader.collectAllMetrics(); - boolean fallbackOccurred = - metrics.stream() - .anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); - - assertFalse("Fallback metric should not be present", fallbackOccurred); - - } finally { - rpc.shutdown(); - } + // Make a call that is expected to fail + SpannerException exception = + assertThrows( + SpannerException.class, + () -> + rpc.executeBatchDml( + com.google.spanner.v1.ExecuteBatchDmlRequest.newBuilder() + .setSession("projects/p/instances/i/databases/d/sessions/s") + .build(), + null)); + assertEquals(ErrorCode.UNAVAILABLE, exception.getErrorCode()); + + // Wait briefly for the 10ms period to trigger the fallback check + Thread.sleep(10); + + // Verify Fallback via Metrics + Collection metrics = metricReader.collectAllMetrics(); + boolean fallbackOccurred = + metrics.stream() + .anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); + + assertFalse("Fallback metric should not be present", fallbackOccurred); + } finally { - SpannerOptions.useDefaultEnvironment(); + rpc.shutdown(); } } @@ -1255,64 +1244,53 @@ public void testFallbackIntegration_switchesToFallbackOnFailure() throws Excepti OpenTelemetrySdk openTelemetry = OpenTelemetrySdk.builder().setMeterProvider(meterProvider).build(); - SpannerOptions.useEnvironment( - new SpannerOptions.SpannerEnvironment() { - @Override - public boolean isEnableGcpFallback() { - return true; - } - }); + SpannerOptions.Builder builder = + SpannerOptions.newBuilder() + .setProjectId("test-project") + .setEnableDirectAccess(true) + .setHost("http://localhost:1") // Closed port + .setCredentials(NoCredentials.getInstance()) + .setOpenTelemetry(openTelemetry); + // Make sure the ExecuteBatchDml RPC fails quickly to keep the test fast. + // Note that the timeout is actually not used. It is the fact that it does not retry that + // makes it fail fast. + builder + .getSpannerStubSettingsBuilder() + .executeBatchDmlSettings() + .setSimpleTimeoutNoRetriesDuration(Duration.ofSeconds(10)); + // Setup Options with invalid host to force error + SpannerOptions options = builder.build(); + + TestableGapicSpannerRpcWithLowerMinFailedCalls rpc = + new TestableGapicSpannerRpcWithLowerMinFailedCalls(options); try { - SpannerOptions.Builder builder = - SpannerOptions.newBuilder() - .setProjectId("test-project") - .setEnableDirectAccess(true) - .setHost("http://localhost:1") // Closed port - .setCredentials(NoCredentials.getInstance()) - .setOpenTelemetry(openTelemetry); - // Make sure the ExecuteBatchDml RPC fails quickly to keep the test fast. - // Note that the timeout is actually not used. It is the fact that it does not retry that - // makes it fail fast. - builder - .getSpannerStubSettingsBuilder() - .executeBatchDmlSettings() - .setSimpleTimeoutNoRetriesDuration(Duration.ofSeconds(10)); - // Setup Options with invalid host to force error - SpannerOptions options = builder.build(); - - TestableGapicSpannerRpcWithLowerMinFailedCalls rpc = - new TestableGapicSpannerRpcWithLowerMinFailedCalls(options); - try { - // Make a call that is expected to fail - SpannerException exception = - assertThrows( - SpannerException.class, - () -> - rpc.executeBatchDml( - com.google.spanner.v1.ExecuteBatchDmlRequest.newBuilder() - .setSession("projects/p/instances/i/databases/d/sessions/s") - .build(), - null)); - assertEquals(ErrorCode.UNAVAILABLE, exception.getErrorCode()); - - // Wait briefly for the 10ms period to trigger the fallback check - Thread.sleep(10); - - // Verify Fallback via Metrics - Collection metrics = metricReader.collectAllMetrics(); - boolean fallbackOccurred = - metrics.stream() - .anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); - - assertTrue( - "Fallback metric should be present, indicating GcpFallbackChannel is active", - fallbackOccurred); - - } finally { - rpc.shutdown(); - } + // Make a call that is expected to fail + SpannerException exception = + assertThrows( + SpannerException.class, + () -> + rpc.executeBatchDml( + com.google.spanner.v1.ExecuteBatchDmlRequest.newBuilder() + .setSession("projects/p/instances/i/databases/d/sessions/s") + .build(), + null)); + assertEquals(ErrorCode.UNAVAILABLE, exception.getErrorCode()); + + // Wait briefly for the 10ms period to trigger the fallback check + Thread.sleep(10); + + // Verify Fallback via Metrics + Collection metrics = metricReader.collectAllMetrics(); + boolean fallbackOccurred = + metrics.stream() + .anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); + + assertTrue( + "Fallback metric should be present, indicating GcpFallbackChannel is active", + fallbackOccurred); + } finally { - SpannerOptions.useDefaultEnvironment(); + rpc.shutdown(); } } From bca51a1e19afd3d4b3fb060d6d189e8079821387 Mon Sep 17 00:00:00 2001 From: kinsaurralde Date: Tue, 7 Apr 2026 22:43:03 +0000 Subject: [PATCH 3/3] formatting fixes --- .../google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java b/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java index f74fbdd3272e..7339d02542e5 100644 --- a/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java +++ b/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java @@ -1201,8 +1201,7 @@ public void testFallbackIntegration_doesNotSwitchWhenThresholdNotMet() throws Ex // Verify Fallback via Metrics Collection metrics = metricReader.collectAllMetrics(); boolean fallbackOccurred = - metrics.stream() - .anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); + metrics.stream().anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); assertFalse("Fallback metric should not be present", fallbackOccurred); @@ -1282,8 +1281,7 @@ public void testFallbackIntegration_switchesToFallbackOnFailure() throws Excepti // Verify Fallback via Metrics Collection metrics = metricReader.collectAllMetrics(); boolean fallbackOccurred = - metrics.stream() - .anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); + metrics.stream().anyMatch(md -> md.getName().contains("fallback_count") && hasValue(md)); assertTrue( "Fallback metric should be present, indicating GcpFallbackChannel is active",