respond to comments

Author: kinsaurralde

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java

@@ -361,16 +361,14 @@ public GapicSpannerRpc(final SpannerOptions options) {
           GrpcTransportOptions.setUpCredentialsProvider(options);
 
       InstantiatingGrpcChannelProvider.Builder defaultChannelProviderBuilder =
-          getDefaultChannelProviderBuilder(
-              options, headerProviderWithUserAgent, isEnableDirectAccess);
+          createChannelProviderBuilder(options, headerProviderWithUserAgent, isEnableDirectAccess);
 
       if (options.getChannelProvider() == null
           && isEnableDirectAccess
           && isEnableGcpFallbackEnv()) {
         InstantiatingGrpcChannelProvider.Builder cloudPathProviderBuilder =
-            getDefaultChannelProviderBuilder(
+            createChannelProviderBuilder(
                 options, headerProviderWithUserAgent, /* isEnableDirectAccess= */ false);
-        cloudPathProviderBuilder.setAttemptDirectPath(false);
 
         final AtomicReference<ManagedChannelBuilder> cloudPathBuilderRef = new AtomicReference<>();
         cloudPathProviderBuilder.setChannelConfigurator(
@@ -612,7 +610,6 @@ GcpFallbackChannelOptions createFallbackChannelOptions(
     return GcpFallbackChannelOptions.newBuilder()
         .setPrimaryChannelName("directpath")
         .setFallbackChannelName("cloudpath")
-        .setMinFailedCalls(1)
         .setGcpFallbackOpenTelemetry(fallbackTelemetry)
         .build();
   }
@@ -626,7 +623,7 @@ private static String parseGrpcGcpApiConfig() {
     }
   }
 
-  private InstantiatingGrpcChannelProvider.Builder getDefaultChannelProviderBuilder(
+  private InstantiatingGrpcChannelProvider.Builder createChannelProviderBuilder(
       final SpannerOptions options,
       final HeaderProvider headerProviderWithUserAgent,
       boolean isEnableDirectAccess) {

google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java

@@ -903,6 +903,74 @@ public TestableGapicSpannerRpc(SpannerOptions options) {
       super(options);
     }
 
+    @Override
+    GcpFallbackChannelOptions createFallbackChannelOptions(
+        GcpFallbackOpenTelemetry fallbackTelemetry) {
+      // Override default 1-minute period to 10ms for instant testing
+      return GcpFallbackChannelOptions.newBuilder()
+          .setPrimaryChannelName("directpath")
+          .setFallbackChannelName("cloudpath")
+          .setPeriod(Duration.ofMillis(10))
+          .setGcpFallbackOpenTelemetry(fallbackTelemetry)
+          .build();
+    }
+  }
+
+  @Test
+  public void testFallbackIntegration_doesNotSwitchWhenThresholdNotMet() throws Exception {
+    GapicSpannerRpc.enableGcpFallbackEnv = true;
+
+    // Setup OpenTelemetry to capture metrics
+    InMemoryMetricReader metricReader = InMemoryMetricReader.create();
+    SdkMeterProvider meterProvider =
+        SdkMeterProvider.builder().registerMetricReader(metricReader).build();
+    OpenTelemetrySdk openTelemetry =
+        OpenTelemetrySdk.builder().setMeterProvider(meterProvider).build();
+
+    // Setup Options with invalid host to force error
+    SpannerOptions options =
+        SpannerOptions.newBuilder()
+            .setProjectId("test-project")
+            .setEnableDirectAccess(true)
+            .setHost("http://localhost:1") // Closed port
+            .setCredentials(NoCredentials.getInstance())
+            .setOpenTelemetry(openTelemetry)
+            .build();
+
+    TestableGapicSpannerRpc rpc = new TestableGapicSpannerRpc(options);
+
+    try {
+      // Make a call that is expected to fail
+      try {
+        rpc.executeBatchDml(
+            com.google.spanner.v1.ExecuteBatchDmlRequest.newBuilder()
+                .setSession("projects/p/instances/i/databases/d/sessions/s")
+                .build(),
+            null);
+      } catch (Exception expected) {
+        // Expect a connection error
+      }
+
+      // Wait briefly for the 10ms period to trigger the fallback check
+      Thread.sleep(100);
+
+      // Verify Fallback via Metrics
+      Collection<MetricData> 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();
+    }
+  }
+
+  static class TestableGapicSpannerRpcWithLowerMinFailedCalls extends GapicSpannerRpc {
+    public TestableGapicSpannerRpcWithLowerMinFailedCalls(SpannerOptions options) {
+      super(options);
+    }
+
     @Override
     GcpFallbackChannelOptions createFallbackChannelOptions(
         GcpFallbackOpenTelemetry fallbackTelemetry) {
@@ -938,7 +1006,8 @@ public void testFallbackIntegration_switchesToFallbackOnFailure() throws Excepti
             .setOpenTelemetry(openTelemetry)
             .build();
 
-    TestableGapicSpannerRpc rpc = new TestableGapicSpannerRpc(options);
+    TestableGapicSpannerRpcWithLowerMinFailedCalls rpc =
+        new TestableGapicSpannerRpcWithLowerMinFailedCalls(options);
 
     try {
       // Make a call that is expected to fail