make dynamic channel pool default disabled

rahul2393 · rahul2393 · commit ce43d4a8bcc4 · 2025-12-16T16:22:56.000+05:30
diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java b/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;
diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/RequestIdMockServerTest.java b/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
diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java b/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)
diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionChannelHintTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionChannelHintTest.java
@@ -24,23 +24,25 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 
+import com.google.api.gax.grpc.GrpcInterceptorProvider;
 import com.google.cloud.NoCredentials;
+import com.google.cloud.grpc.GcpManagedChannel;
 import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult;
 import com.google.cloud.spanner.Options.RpcPriority;
 import com.google.cloud.spanner.ReadContext.QueryAnalyzeMode;
+import com.google.common.collect.ImmutableList;
 import com.google.protobuf.ListValue;
 import com.google.spanner.v1.ResultSetMetadata;
 import com.google.spanner.v1.SpannerGrpc;
 import com.google.spanner.v1.StructType;
 import com.google.spanner.v1.StructType.Field;
 import com.google.spanner.v1.TypeCode;
-import io.grpc.Context;
-import io.grpc.Contexts;
-import io.grpc.Metadata;
+import io.grpc.CallOptions;
+import io.grpc.Channel;
+import io.grpc.ClientCall;
+import io.grpc.ClientInterceptor;
+import io.grpc.MethodDescriptor;
 import io.grpc.Server;
-import io.grpc.ServerCall;
-import io.grpc.ServerCallHandler;
-import io.grpc.ServerInterceptor;
 import io.grpc.netty.shaded.io.grpc.netty.NettyServerBuilder;
 import java.net.InetSocketAddress;
 import java.util.Set;
@@ -93,10 +95,11 @@ public class TransactionChannelHintTest {
   private static MockSpannerServiceImpl mockSpanner;
   private static Server server;
   private static InetSocketAddress address;
-  // Track channel hints (from X-Goog-Spanner-Request-Id header) per RPC method
-  private static final Set<Long> executeSqlChannelHints = ConcurrentHashMap.newKeySet();
-  private static final Set<Long> beginTransactionChannelHints = ConcurrentHashMap.newKeySet();
-  private static final Set<Long> streamingReadChannelHints = ConcurrentHashMap.newKeySet();
+  // Track logical affinity keys (before grpc-gcp routing) per RPC method.
+  // These are captured by a client interceptor to verify channel affinity consistency.
+  private static final Set<String> executeSqlAffinityKeys = ConcurrentHashMap.newKeySet();
+  private static final Set<String> beginTransactionAffinityKeys = ConcurrentHashMap.newKeySet();
+  private static final Set<String> streamingReadAffinityKeys = ConcurrentHashMap.newKeySet();
   private static Level originalLogLevel;
 
   @BeforeClass
@@ -109,49 +112,40 @@ public static void startServer() throws Exception {
         StatementResult.query(READ_ONE_KEY_VALUE_STATEMENT, READ_ONE_KEY_VALUE_RESULTSET));
 
     address = new InetSocketAddress("localhost", 0);
-    server =
-        NettyServerBuilder.forAddress(address)
-            .addService(mockSpanner)
-            // Add a server interceptor to extract channel hints from X-Goog-Spanner-Request-Id
-            // header. This verifies that all operations in a transaction use the same channel hint.
-            .intercept(
-                new ServerInterceptor() {
-                  @Override
-                  public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
-                      ServerCall<ReqT, RespT> call,
-                      Metadata headers,
-                      ServerCallHandler<ReqT, RespT> next) {
-                    // Extract channel hint from X-Goog-Spanner-Request-Id header
-                    String requestId = headers.get(XGoogSpannerRequestId.REQUEST_ID_HEADER_KEY);
-                    if (requestId != null) {
-                      // Format:
-                      // <version>.<randProcessId>.<nthClientId>.<nthChannelId>.<nthRequest>.<attempt>
-                      String[] parts = requestId.split("\\.");
-                      if (parts.length >= 4) {
-                        try {
-                          long channelHint = Long.parseLong(parts[3]);
-                          if (call.getMethodDescriptor()
-                              .equals(SpannerGrpc.getExecuteStreamingSqlMethod())) {
-                            executeSqlChannelHints.add(channelHint);
-                          }
-                          if (call.getMethodDescriptor()
-                              .equals(SpannerGrpc.getStreamingReadMethod())) {
-                            streamingReadChannelHints.add(channelHint);
-                          }
-                          if (call.getMethodDescriptor()
-                              .equals(SpannerGrpc.getBeginTransactionMethod())) {
-                            beginTransactionChannelHints.add(channelHint);
-                          }
-                        } catch (NumberFormatException e) {
-                          // Ignore parse errors
-                        }
-                      }
-                    }
-                    return Contexts.interceptCall(Context.current(), call, headers, next);
+    server = NettyServerBuilder.forAddress(address).addService(mockSpanner).build().start();
+  }
+
+  /**
+   * Creates a client interceptor that captures the logical affinity key before grpc-gcp routes the
+   * request. This allows us to verify that all operations within a transaction use the same logical
+   * channel affinity, even though the physical channel ID may vary.
+   */
+  private static GrpcInterceptorProvider createAffinityKeyInterceptorProvider() {
+    return () ->
+        ImmutableList.of(
+            new ClientInterceptor() {
+              @Override
+              public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
+                  MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) {
+                // Capture the AFFINITY_KEY before grpc-gcp processes it
+                String affinityKey = callOptions.getOption(GcpManagedChannel.AFFINITY_KEY);
+                if (affinityKey != null) {
+                  String methodName = method.getFullMethodName();
+                  if (methodName.equals(
+                      SpannerGrpc.getExecuteStreamingSqlMethod().getFullMethodName())) {
+                    executeSqlAffinityKeys.add(affinityKey);
+                  }
+                  if (methodName.equals(SpannerGrpc.getStreamingReadMethod().getFullMethodName())) {
+                    streamingReadAffinityKeys.add(affinityKey);
+                  }
+                  if (methodName.equals(
+                      SpannerGrpc.getBeginTransactionMethod().getFullMethodName())) {
+                    beginTransactionAffinityKeys.add(affinityKey);
                   }
-                })
-            .build()
-            .start();
+                }
+                return next.newCall(method, callOptions);
+              }
+            });
   }
 
   @AfterClass
@@ -176,9 +170,9 @@ public static void resetLogging() {
   @After
   public void reset() {
     mockSpanner.reset();
-    executeSqlChannelHints.clear();
-    streamingReadChannelHints.clear();
-    beginTransactionChannelHints.clear();
+    executeSqlAffinityKeys.clear();
+    streamingReadAffinityKeys.clear();
+    beginTransactionAffinityKeys.clear();
   }
 
   private SpannerOptions createSpannerOptions() {
@@ -193,6 +187,7 @@ private SpannerOptions createSpannerOptions() {
         .setCompressorName("gzip")
         .setHost("http://" + endpoint)
         .setCredentials(NoCredentials.getInstance())
+        .setInterceptorProvider(createAffinityKeyInterceptorProvider())
         .setSessionPoolOption(
             SessionPoolOptions.newBuilder().setSkipVerifyingBeginTransactionForMuxRW(true).build())
         .build();
@@ -206,7 +201,8 @@ public void testSingleUseReadOnlyTransaction_usesSingleChannelHint() {
         while (resultSet.next()) {}
       }
     }
-    assertEquals(1, executeSqlChannelHints.size());
+    // All ExecuteSql calls should use the same logical affinity key
+    assertEquals(1, executeSqlAffinityKeys.size());
   }
 
   @Test
@@ -220,7 +216,8 @@ public void testSingleUseReadOnlyTransaction_withTimestampBound_usesSingleChanne
         while (resultSet.next()) {}
       }
     }
-    assertEquals(1, executeSqlChannelHints.size());
+    // All ExecuteSql calls should use the same logical affinity key
+    assertEquals(1, executeSqlAffinityKeys.size());
   }
 
   @Test
@@ -236,10 +233,10 @@ public void testReadOnlyTransaction_usesSingleChannelHint() {
         }
       }
     }
-    // All ExecuteSql calls within the transaction should use the same channel hint
-    assertEquals(1, executeSqlChannelHints.size());
-    // BeginTransaction should use a single channel hint
-    assertEquals(1, beginTransactionChannelHints.size());
+    // All ExecuteSql calls within the transaction should use the same logical affinity key
+    assertEquals(1, executeSqlAffinityKeys.size());
+    // BeginTransaction should use a single logical affinity key
+    assertEquals(1, beginTransactionAffinityKeys.size());
   }
 
   @Test
@@ -256,10 +253,10 @@ public void testReadOnlyTransaction_withTimestampBound_usesSingleChannelHint() {
         }
       }
     }
-    // All ExecuteSql calls within the transaction should use the same channel hint
-    assertEquals(1, executeSqlChannelHints.size());
-    // BeginTransaction should use a single channel hint
-    assertEquals(1, beginTransactionChannelHints.size());
+    // All ExecuteSql calls within the transaction should use the same logical affinity key
+    assertEquals(1, executeSqlAffinityKeys.size());
+    // BeginTransaction should use a single logical affinity key
+    assertEquals(1, beginTransactionAffinityKeys.size());
   }
 
   @Test
@@ -288,7 +285,8 @@ public void testTransactionManager_usesSingleChannelHint() {
         }
       }
     }
-    assertEquals(1, executeSqlChannelHints.size());
+    // All ExecuteSql calls within the transaction should use the same logical affinity key
+    assertEquals(1, executeSqlAffinityKeys.size());
   }
 
   @Test
@@ -318,6 +316,7 @@ public void testTransactionRunner_usesSingleChannelHint() {
             return null;
           });
     }
-    assertEquals(1, streamingReadChannelHints.size());
+    // All StreamingRead calls within the transaction should use the same logical affinity key
+    assertEquals(1, streamingReadAffinityKeys.size());
   }
 }
diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/RequestIdInterceptorTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/RequestIdInterceptorTest.java
