incorporate suggestions

Author: rahul2393

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

@@ -163,16 +163,16 @@ public class SpannerOptions extends ServiceOptions<Spanner, SpannerOptions> {
 
   /**
    * Default affinity key lifetime for dynamic channel pool. This is how long to keep an affinity
-   * key after its last use. Zero means keeping keys forever. Default is 1 hour.
+   * key after its last use. Zero means keeping keys forever. Default is 10 minutes, which is
+   * sufficient to ensure that requests within a single transaction use the same channel.
    */
-  public static final Duration DEFAULT_DYNAMIC_POOL_AFFINITY_KEY_LIFETIME = Duration.ofHours(1);
+  public static final Duration DEFAULT_DYNAMIC_POOL_AFFINITY_KEY_LIFETIME = Duration.ofMinutes(10);
 
   /**
    * Default cleanup interval for dynamic channel pool affinity keys. This is how frequently the
-   * affinity key cleanup process runs. Default is 6 minutes (1/10 of default affinity key
-   * lifetime).
+   * affinity key cleanup process runs. Default is 1 minute (1/10 of default affinity key lifetime).
    */
-  public static final Duration DEFAULT_DYNAMIC_POOL_CLEANUP_INTERVAL = Duration.ofMinutes(6);
+  public static final Duration DEFAULT_DYNAMIC_POOL_CLEANUP_INTERVAL = Duration.ofMinutes(1);
 
   private final TransportChannelProvider channelProvider;
 
@@ -915,6 +915,14 @@ protected SpannerOptions(Builder builder) {
       dynamicPoolCleanupInterval = DEFAULT_DYNAMIC_POOL_CLEANUP_INTERVAL;
     }
 
+    // Validate that if affinity key lifetime is set (non-zero), cleanup interval must be positive.
+    // A zero cleanup interval with a non-zero affinity key lifetime would disable cleanup of stale
+    // affinity keys, potentially leading to a memory leak.
+    Preconditions.checkArgument(
+        dynamicPoolAffinityKeyLifetime.isZero() || !dynamicPoolCleanupInterval.isZero(),
+        "Cleanup interval must be positive when affinity key lifetime is set, got cleanup interval: %s",
+        dynamicPoolCleanupInterval);
+
     autoThrottleAdministrativeRequests = builder.autoThrottleAdministrativeRequests;
     retryAdministrativeRequestsSettings = builder.retryAdministrativeRequestsSettings;
     trackTransactionStarter = builder.trackTransactionStarter;
@@ -1838,7 +1846,7 @@ public Builder setDynamicPoolAffinityKeyLifetime(Duration lifetime) {
 
     /**
      * Sets the cleanup interval for the dynamic channel pool affinity keys. This determines how
-     * frequently the affinity key cleanup process runs. Default is 6 minutes. Must be positive if
+     * frequently the affinity key cleanup process runs. Default is 1 minute. Must be positive if
      * affinity key lifetime is set.
      *
      * <p>This setting is only effective when dynamic channel pooling is enabled.
@@ -2310,13 +2318,13 @@ public int getDynamicPoolMinChannels() {
 
   /**
    * Returns the affinity key lifetime for the dynamic pool. This is how long to keep an affinity
-   * key after its last use. Default is 1 hour.
+   * key after its last use. Default is 10 minutes.
    */
   public Duration getDynamicPoolAffinityKeyLifetime() {
     return dynamicPoolAffinityKeyLifetime;
   }
 
-  /** Returns the cleanup interval for dynamic pool affinity keys. Default is 6 minutes. */
+  /** Returns the cleanup interval for dynamic pool affinity keys. Default is 1 minute. */
   public Duration getDynamicPoolCleanupInterval() {
     return dynamicPoolCleanupInterval;
   }

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

@@ -597,7 +597,9 @@ private static GcpManagedChannelOptions grpcGcpOptionsWithMetricsAndDcp(SpannerO
 
     // Configure dynamic channel pool options if enabled
     if (options.isDynamicChannelPoolEnabled()) {
-      GcpChannelPoolOptions existingPoolOptions = grpcGcpOptions.getChannelPoolOptions();
+      GcpChannelPoolOptions existingPoolOptions =
+          MoreObjects.firstNonNull(
+              grpcGcpOptions.getChannelPoolOptions(), GcpChannelPoolOptions.newBuilder().build());
       GcpChannelPoolOptions.Builder poolOptionsBuilder =
           GcpChannelPoolOptions.newBuilder(existingPoolOptions);
 

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

@@ -16,7 +16,6 @@
 
 package com.google.cloud.spanner;
 
-import static io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
@@ -33,23 +32,15 @@
 import com.google.spanner.v1.BatchCreateSessionsRequest;
 import com.google.spanner.v1.BeginTransactionRequest;
 import com.google.spanner.v1.ExecuteSqlRequest;
-import io.grpc.Attributes;
 import io.grpc.CallOptions;
 import io.grpc.Channel;
 import io.grpc.ClientCall;
 import io.grpc.ClientInterceptor;
 import io.grpc.Context;
 import io.grpc.Deadline;
 import io.grpc.ManagedChannelBuilder;
-import io.grpc.Metadata;
 import io.grpc.MethodDescriptor;
-import io.grpc.ServerCall;
-import io.grpc.ServerCall.Listener;
-import io.grpc.ServerCallHandler;
-import io.grpc.ServerInterceptor;
 import io.grpc.Status;
-import java.io.IOException;
-import java.net.InetSocketAddress;
 import java.time.Duration;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -70,18 +61,14 @@
 
 @RunWith(JUnit4.class)
 public class RetryOnDifferentGrpcChannelMockServerTest extends AbstractMockServerTest {
-  private static final Map<String, Set<InetSocketAddress>> SERVER_ADDRESSES = new HashMap<>();
-
-  /** Tracks the physical channel IDs from request ID headers (set by grpc-gcp). */
-  private static final Map<String, Set<Long>> CHANNEL_HINTS = new HashMap<>();
-
   /** Tracks the logical affinity keys before grpc-gcp routes the request. */
   private static final Map<String, Set<String>> LOGICAL_AFFINITY_KEYS = new HashMap<>();
 
   @BeforeClass
-  public static void startStaticServer() throws IOException {
+  public static void setupAndStartServer() throws Exception {
     System.setProperty("spanner.retry_deadline_exceeded_on_different_channel", "true");
-    startStaticServer(createServerInterceptor());
+    // Call the parent's startStaticServer to set up the mock server
+    AbstractMockServerTest.startStaticServer();
   }
 
   @AfterClass
@@ -91,8 +78,6 @@ public static void removeSystemProperty() {
 
   @After
   public void clearRequests() {
-    SERVER_ADDRESSES.clear();
-    CHANNEL_HINTS.clear();
     LOGICAL_AFFINITY_KEYS.clear();
     mockSpanner.clearRequests();
     mockSpanner.removeAllExecutionTimes();
@@ -125,53 +110,6 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
             });
   }
 
-  static ServerInterceptor createServerInterceptor() {
-    return new ServerInterceptor() {
-      @Override
-      public <ReqT, RespT> Listener<ReqT> interceptCall(
-          ServerCall<ReqT, RespT> serverCall,
-          Metadata metadata,
-          ServerCallHandler<ReqT, RespT> serverCallHandler) {
-        Attributes attributes = serverCall.getAttributes();
-        String methodName = serverCall.getMethodDescriptor().getFullMethodName();
-        //noinspection unchecked,deprecation
-        Attributes.Key<InetSocketAddress> key =
-            (Attributes.Key<InetSocketAddress>)
-                attributes.keys().stream()
-                    .filter(k -> k.equals(TRANSPORT_ATTR_REMOTE_ADDR))
-                    .findFirst()
-                    .orElse(null);
-        if (key != null) {
-          InetSocketAddress address = attributes.get(key);
-          synchronized (SERVER_ADDRESSES) {
-            Set<InetSocketAddress> addresses =
-                SERVER_ADDRESSES.getOrDefault(methodName, new HashSet<>());
-            addresses.add(address);
-            SERVER_ADDRESSES.putIfAbsent(methodName, addresses);
-          }
-        }
-        String requestId = metadata.get(XGoogSpannerRequestId.REQUEST_ID_HEADER_KEY);
-        if (requestId != null) {
-          // REQUEST_ID format: version.randProcessId.nthClientId.nthChannelId.nthRequest.attempt
-          String[] parts = requestId.split("\\.");
-          if (parts.length >= 6) {
-            try {
-              long channelHint = Long.parseLong(parts[3]);
-              synchronized (CHANNEL_HINTS) {
-                Set<Long> hints = CHANNEL_HINTS.getOrDefault(methodName, new HashSet<>());
-                hints.add(channelHint);
-                CHANNEL_HINTS.putIfAbsent(methodName, hints);
-              }
-            } catch (NumberFormatException ignore) {
-              // Ignore malformed header values in tests.
-            }
-          }
-        }
-        return serverCallHandler.startCall(serverCall, metadata);
-      }
-    };
-  }
-
   SpannerOptions.Builder createSpannerOptionsBuilder() {
     return SpannerOptions.newBuilder()
         .setProjectId("my-project")