feat(grpc-gcp): Add shared fallback state and probing recovery options to GcpFallbackChannel - #14013
feat(grpc-gcp): Add shared fallback state and probing recovery options to GcpFallbackChannel#14013kinsaurralde wants to merge 12 commits into
Conversation
…ns to GcpFallbackChannel
There was a problem hiding this comment.
Code Review
This pull request introduces GcpFallbackState to enable coordinated pool-wide failover, recovery, and background task management across multiple channels, along with options for per-channel independent recovery. The review feedback highlights critical issues in the state machine and probing logic: specifically, localInFallbackMode is not properly reset when transitioning out of fallback mode under pool-level recovery, which can corrupt probing statistics on subsequent failovers. Additionally, a potential race condition in probePrimary() could cause localFirstPrimaryProbeSuccessNanos to be read as zero, prematurely satisfying the recovery duration check.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared state mechanism (GcpFallbackState) to coordinate pool-wide failover, recovery, and background tasks across multiple gRPC channels, along with options for per-channel recovery. The review feedback highlights several critical concurrency and thread-safety issues, including a performance bottleneck and race condition on the hot path in isInFallbackMode(), a race condition in startPeriodicEvaluation that could cause tasks to run after shutdown, a resource management bug where externally-provided executors might be improperly shut down, and an observability issue where telemetry is only captured for the first channel.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces GcpFallbackState to manage shared, thread-safe state across multiple channels in a pool, enabling coordinated failover, recovery, and background task execution. It also adds new configuration options to GcpFallbackChannelOptions (such as per-channel recovery and shared executor services) and includes comprehensive unit tests. The review feedback identifies two key areas for improvement: first, the checkErrorRates() method in GcpFallbackChannel has become dead code and should be removed; second, the implementation of notifyWhenStateChanged does not handle transitions between primary and fallback modes, which could cause callers to miss critical connectivity state changes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces GcpFallbackState to enable coordinated pool-wide failover, recovery, and shared background task execution across multiple channels. It updates GcpFallbackChannel and GcpFallbackChannelOptions to support shared state, per-channel recovery, and configurable recovery thresholds, backed by comprehensive unit tests. The review feedback highlights two key issues: first, starting periodic evaluation can get permanently stuck in an uninitialized state if the executor service is null or shutdown; second, pending connectivity state callbacks registered via notifyWhenStateChanged may be orphaned when transitioning between primary and fallback channels.
Moves stat collection used to determine error rates for fallback from GcpFallbackChannel to GcpFallbackState.
Adds the ability for the probing function to switch the channel back to the primary channel from fallback if thresholds are met.
This is in preparation of spanner adding a probe and recovery policy. Currently in spanner, GcpFallbackChannel is at the pool level. However for probing and recovery we need each channel to be probed. This shared state allows switching GcpFallbackChannel to be at the channel level (with each object holding 1 directpath and 1 cloudpath) while having the fallback policy move all channels together
Using probes for recovery is an option that will be disabled by default to match current condition.