Skip to content

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

Draft
kinsaurralde wants to merge 3 commits into
googleapis:mainfrom
kinsaurralde:gcp_fallback_shared_state
Draft

feat(grpc-gcp): Add shared fallback state and probing recovery options to GcpFallbackChannel#14013
kinsaurralde wants to merge 3 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.

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.

1 participant