Skip to content

feat(grpc-gcp): Add shared fallback state and probing recovery options to GcpFallbackChannel - #14013

Open
kinsaurralde wants to merge 12 commits into
googleapis:mainfrom
kinsaurralde:gcp_fallback_shared_state
Open

feat(grpc-gcp): Add shared fallback state and probing recovery options to GcpFallbackChannel#14013
kinsaurralde wants to merge 12 commits into
googleapis:mainfrom
kinsaurralde:gcp_fallback_shared_state

Conversation

@kinsaurralde

Copy link
Copy Markdown
Contributor
  1. Moves stat collection used to determine error rates for fallback from GcpFallbackChannel to GcpFallbackState.

  2. 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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kinsaurralde
kinsaurralde marked this pull request as ready for review August 11, 2026 00:22
@kinsaurralde
kinsaurralde requested review from a team as code owners August 11, 2026 00:22
@rahul2393

Copy link
Copy Markdown
Contributor

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread grpc-gcp-java/src/main/java/com/google/cloud/grpc/fallback/GcpFallbackState.java Outdated
Comment thread grpc-gcp-java/src/main/java/com/google/cloud/grpc/fallback/GcpFallbackState.java Outdated
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2026
@rahul2393

Copy link
Copy Markdown
Contributor

/gemini review

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2026
@rahul2393

Copy link
Copy Markdown
Contributor

/gemini review

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants