test(spanner): unflake LocationAwareSharedBackendReplicaHarnessTest - #14396
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces connection tracking for replicas in SharedBackendReplicaHarness to ensure all replicas are connected before executing tests in LocationAwareSharedBackendReplicaHarnessTest. This is achieved by adding a ServerTransportFilter to the gRPC server and tracking active connections with an AtomicInteger. The review feedback suggests a robust improvement to prevent the activeConnections counter from dropping below zero in recordConnectionTerminated if a transport terminates without a successful handshake, which could otherwise lead to incorrect connection state tracking.
| void recordConnectionTerminated() { | ||
| activeConnections.decrementAndGet(); | ||
| } |
There was a problem hiding this comment.
Under certain network conditions or connection handshake failures, gRPC's transportTerminated can be invoked without a corresponding successful transportReady call. If this occurs, activeConnections could decrement below zero, causing subsequent successful connections to not be recognized correctly (since activeConnections.get() > 0 would remain false or delayed). To make the connection tracking robust against such anomalies, consider using updateAndGet to prevent the counter from dropping below zero.
| void recordConnectionTerminated() { | |
| activeConnections.decrementAndGet(); | |
| } | |
| void recordConnectionTerminated() { | |
| activeConnections.updateAndGet(val -> Math.max(0, val - 1)); | |
| } |
| if (harness.allReplicasConnected()) { | ||
| return; | ||
| } | ||
| Thread.sleep(20L); |
There was a problem hiding this comment.
nit: we could use awaitility here
There was a problem hiding this comment.
Yes org.awaitility.Awaitility.await
Wait for all backend replicas to establish active transport connections before issuing the warmup query in waitForReplicaRoutedRead. Previously, waitForReplicaRoutedRead exited as soon as the first replica handled the initial warmup read. In fast test executions, secondary replicas could still be completing their background Netty channel handshakes initiated by EndpointLifecycleManager probing. When the primary replica subsequently failed, secondary replicas were evaluated as unhealthy (channel state not yet READY) and skipped, causing traffic to unexpectedly fall back to defaultReplica. Tracking active transport connections via ServerTransportFilter in SharedBackendReplicaHarness ensures all replica channels are connected and ready before test assertions begin.
0e8b6fc to
ba70900
Compare
2 participants
Wait for all backend replicas to establish active transport connections before issuing the warmup query in waitForReplicaRoutedRead.
Previously, waitForReplicaRoutedRead exited as soon as the first replica handled the initial warmup read. In fast test executions, secondary replicas could still be completing their background Netty channel handshakes initiated by EndpointLifecycleManager probing. When the primary replica subsequently failed, secondary replicas were evaluated as unhealthy (channel state not yet READY) and skipped, causing traffic to unexpectedly fall back to defaultReplica.
Tracking active transport connections via ServerTransportFilter in SharedBackendReplicaHarness ensures all replica channels are connected and ready before test assertions begin.