make dynamic channel pool default disabled

Author: rahul2393

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java

@@ -850,15 +850,17 @@ protected SpannerOptions(Builder builder) {
     grpcGcpExtensionEnabled = builder.grpcGcpExtensionEnabled;
     grpcGcpOptions = builder.grpcGcpOptions;
 
-    // Dynamic channel pooling is enabled by default when:
-    // 1. grpc-gcp extension is enabled, AND
-    // 2. numChannels is not explicitly set, AND
-    // 3. dynamicChannelPoolEnabled is not explicitly set to false
-    if (builder.dynamicChannelPoolEnabled != null) {
-      dynamicChannelPoolEnabled = builder.dynamicChannelPoolEnabled && grpcGcpExtensionEnabled;
-    } else {
-      // Enable DCP by default only if grpc-gcp is enabled and numChannels was not explicitly set
+    // Dynamic channel pooling is disabled by default.
+    // It is only enabled when:
+    // 1. enableDynamicChannelPool() was explicitly called, AND
+    // 2. grpc-gcp extension is enabled, AND
+    // 3. numChannels was not explicitly set
+    if (builder.dynamicChannelPoolEnabled != null && builder.dynamicChannelPoolEnabled) {
+      // DCP was explicitly enabled, but respect numChannels if set
       dynamicChannelPoolEnabled = grpcGcpExtensionEnabled && !builder.numChannelsExplicitlySet;
+    } else {
+      // DCP is disabled by default, or was explicitly disabled
+      dynamicChannelPoolEnabled = false;
     }
 
     // Use defaults with proper bounds checking for DCP configuration
@@ -1722,12 +1724,25 @@ public Builder disableGrpcGcpExtension() {
       return this;
     }
 
+    /**
+     * Enables dynamic channel pooling. When enabled, the client will automatically scale the number
+     * of channels based on load. This requires the gRPC-GCP extension to be enabled.
+     *
+     * <p>Dynamic channel pooling is disabled by default. Use this method to explicitly enable it.
+     * Note that calling {@link #setNumChannels(int)} will disable dynamic channel pooling even if
+     * this method was called.
+     */
+    public Builder enableDynamicChannelPool() {
+      this.dynamicChannelPoolEnabled = true;
+      return this;
+    }
+
     /**
      * Disables dynamic channel pooling. When disabled, the client will use a static number of
      * channels as configured by {@link #setNumChannels(int)}.
      *
-     * <p>Dynamic channel pooling is enabled by default unless {@link #setNumChannels(int)} is
-     * called or this method is used to disable it.
+     * <p>Dynamic channel pooling is disabled by default, so this method is typically not needed
+     * unless you want to explicitly disable it after enabling it.
      */
     public Builder disableDynamicChannelPool() {
       this.dynamicChannelPoolEnabled = false;
@@ -2273,9 +2288,10 @@ public GcpManagedChannelOptions getGrpcGcpOptions() {
   }
 
   /**
-   * Returns whether dynamic channel pooling is enabled. Dynamic channel pooling is enabled by
-   * default unless {@link Builder#setNumChannels(int)} is called or {@link
-   * Builder#disableDynamicChannelPool()} is used.
+   * Returns whether dynamic channel pooling is enabled. Dynamic channel pooling is disabled by
+   * default. Use {@link Builder#enableDynamicChannelPool()} to explicitly enable it. Note that
+   * calling {@link Builder#setNumChannels(int)} will disable dynamic channel pooling even if it was
+   * explicitly enabled.
    */
   public boolean isDynamicChannelPoolEnabled() {
     return dynamicChannelPoolEnabled;

google-cloud-spanner/src/test/java/com/google/cloud/spanner/RequestIdMockServerTest.java

@@ -533,7 +533,7 @@ public void testPartitionedDml() {
             XGoogSpannerRequestId.of(getClientId(), -1, 1, 1),
             XGoogSpannerRequestId.of(getClientId(), -1, 2, 1)),
         actual);
-    verifySameChannelId(actual);
+    // Channel ID is determined by grpc-gcp, so we don't verify same channel ID here.
   }
 
   @Test
@@ -559,7 +559,7 @@ public void testPartitionedDmlError() {
             XGoogSpannerRequestId.of(getClientId(), -1, 1, 1),
             XGoogSpannerRequestId.of(getClientId(), -1, 2, 1)),
         actual);
-    verifySameChannelId(actual);
+    // Channel ID is determined by grpc-gcp, so we don't verify same channel ID here.
     assertEquals(XGoogSpannerRequestId.of(exception.getRequestId()), actual.get(1));
   }
 
@@ -586,7 +586,7 @@ public void testPartitionedDmlAborted() {
             XGoogSpannerRequestId.of(getClientId(), -1, 3, 1),
             XGoogSpannerRequestId.of(getClientId(), -1, 4, 1)),
         actual);
-    verifySameChannelId(actual);
+    // Channel ID is determined by grpc-gcp, so we don't verify same channel ID here.
   }
 
   @Test
@@ -612,7 +612,7 @@ public void testPartitionedDmlUnavailable() {
             XGoogSpannerRequestId.of(getClientId(), -1, 3, 1),
             XGoogSpannerRequestId.of(getClientId(), -1, 4, 1)),
         actual);
-    verifySameChannelId(actual);
+    // Channel ID is determined by grpc-gcp, so we don't verify same channel ID here.
   }
 
   @Test
@@ -648,7 +648,7 @@ public void testPartitionedDmlUnavailableWithResumeToken() {
             XGoogSpannerRequestId.of(getClientId(), -1, 2, 1),
             XGoogSpannerRequestId.of(getClientId(), -1, 2, 2)),
         actual);
-    verifySameChannelId(actual);
+    // Channel ID is determined by grpc-gcp, so we don't verify same channel ID here.
   }
 
   @Test

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java

@@ -1203,12 +1203,22 @@ public void testExperimentalHostOptions() {
   }
 
   @Test
-  public void testDynamicChannelPoolingEnabledByDefault() {
+  public void testDynamicChannelPoolingDisabledByDefault() {
     SpannerOptions options =
         SpannerOptions.newBuilder()
             .setProjectId("test-project")
             .setCredentials(NoCredentials.getInstance())
-            .enableGrpcGcpExtension()
+            .build();
+    assertFalse(options.isDynamicChannelPoolEnabled());
+  }
+
+  @Test
+  public void testDynamicChannelPoolingEnabledExplicitly() {
+    SpannerOptions options =
+        SpannerOptions.newBuilder()
+            .setProjectId("test-project")
+            .setCredentials(NoCredentials.getInstance())
+            .enableDynamicChannelPool()
             .build();
     assertTrue(options.isDynamicChannelPoolEnabled());
     assertEquals(
@@ -1230,7 +1240,7 @@ public void testDynamicChannelPoolingDisabledWhenNumChannelsSet() {
         SpannerOptions.newBuilder()
             .setProjectId("test-project")
             .setCredentials(NoCredentials.getInstance())
-            .enableGrpcGcpExtension()
+            .enableDynamicChannelPool()
             .setNumChannels(5) // Explicitly setting numChannels should disable DCP.
             .build();
     assertFalse(options.isDynamicChannelPoolEnabled());
@@ -1243,7 +1253,7 @@ public void testDynamicChannelPoolingDisabledExplicitly() {
         SpannerOptions.newBuilder()
             .setProjectId("test-project")
             .setCredentials(NoCredentials.getInstance())
-            .enableGrpcGcpExtension()
+            .enableDynamicChannelPool()
             .disableDynamicChannelPool()
             .build();
     assertFalse(options.isDynamicChannelPoolEnabled());
@@ -1256,7 +1266,7 @@ public void testDynamicChannelPoolingCustomSettings() {
         SpannerOptions.newBuilder()
             .setProjectId("test-project")
             .setCredentials(NoCredentials.getInstance())
-            .enableGrpcGcpExtension()
+            .enableDynamicChannelPool()
             .setDynamicPoolInitialSize(6)
             .setDynamicPoolMaxChannels(15)
             .setDynamicPoolMinChannels(3)

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

@@ -182,6 +182,56 @@ public void start(Listener<RespT> responseListener, Metadata headers) {
         channelIdStr.equals(String.valueOf(originalChannelId)));
   }
 
+  @Test
+  public void testInterceptorOverridesChannelIdWhenGrpcGcpProvides() {
+    RequestIdInterceptor interceptor = new RequestIdInterceptor();
+
+    // Start with a non-zero channel ID.
+    long originalChannelId = 3;
+    XGoogSpannerRequestId requestId = XGoogSpannerRequestId.of(1, originalChannelId, 5, 0);
+
+    // Simulate grpc-gcp setting a different channel ID.
+    int gcpChannelId = 7;
+    CallOptions callOptions =
+        CallOptions.DEFAULT
+            .withOption(REQUEST_ID_CALL_OPTIONS_KEY, requestId)
+            .withOption(GcpManagedChannel.CHANNEL_ID_KEY, gcpChannelId);
+
+    AtomicReference<Metadata> capturedHeaders = new AtomicReference<>();
+
+    Channel fakeChannel =
+        new FakeChannel() {
+          @Override
+          public <ReqT, RespT> ClientCall<ReqT, RespT> newCall(
+              MethodDescriptor<ReqT, RespT> methodDescriptor, CallOptions callOptions) {
+            return new FakeClientCall<ReqT, RespT>() {
+              @Override
+              public void start(Listener<RespT> responseListener, Metadata headers) {
+                capturedHeaders.set(headers);
+              }
+            };
+          }
+        };
+
+    MethodDescriptor<String, String> methodDescriptor = createMethodDescriptor();
+    ClientCall<String, String> call =
+        interceptor.interceptCall(methodDescriptor, callOptions, fakeChannel);
+    call.start(new NoOpListener<>(), new Metadata());
+
+    assertNotNull(capturedHeaders.get());
+    String headerValue = capturedHeaders.get().get(REQUEST_ID_HEADER_KEY);
+    assertNotNull(headerValue);
+
+    // Parse the header and verify the channel ID WAS updated to grpc-gcp's value.
+    Matcher matcher = REQUEST_ID_PATTERN.matcher(headerValue);
+    assertTrue("Header value should match request ID pattern", matcher.matches());
+    String channelIdStr = matcher.group(4);
+    // Channel ID should be gcpChannelId + 1 = 8 (grpc-gcp's channel ID overrides the original).
+    assertTrue(
+        "Channel ID should be " + (gcpChannelId + 1) + " but was " + channelIdStr,
+        channelIdStr.equals(String.valueOf(gcpChannelId + 1)));
+  }
+
   @Test
   public void testInterceptorWithNoRequestId() {
     RequestIdInterceptor interceptor = new RequestIdInterceptor();