Skip to content

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

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

kinsaurralde wants to merge 18 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.

@kinsaurralde
kinsaurralde force-pushed the gcp_fallback_shared_state branch from c44f3b5 to a183b5c Compare September 17, 2026 02:27
@kinsaurralde
kinsaurralde force-pushed the gcp_fallback_shared_state branch from a183b5c to bfce1d5 Compare September 17, 2026 03:17
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2026
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 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 17, 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 coordinate failover, recovery, and periodic error evaluation across a pool of GcpFallbackChannel instances, along with corresponding updates to GcpFallbackChannelOptions and comprehensive unit tests. The review feedback highlights two key areas for improvement in GcpFallbackState: handling potential RejectedExecutionException when scheduling periodic tasks to prevent the evaluationStarted flag from getting stuck, and addressing design concerns where differing configurations on shared channels are silently ignored.

Comment on lines +165 to +170
scheduledEvaluationFuture =
executor.scheduleAtFixedRate(
() -> checkErrorRates(options, openTelemetry),
options.getPeriod().toMillis(),
options.getPeriod().toMillis(),
TimeUnit.MILLISECONDS);

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.

medium

If executor.scheduleAtFixedRate throws a RejectedExecutionException (e.g., if the executor is saturated or shutting down), the evaluationStarted flag will remain stuck at true. This prevents any subsequent attempts to start the periodic evaluation loop if the failure is transient or handled.

Wrapping the scheduling call in a try-catch block to reset evaluationStarted to false on failure ensures the state remains consistent.

      try {
        scheduledEvaluationFuture =
            executor.scheduleAtFixedRate(
                () -> checkErrorRates(options, openTelemetry),
                options.getPeriod().toMillis(),
                options.getPeriod().toMillis(),
                TimeUnit.MILLISECONDS);
      } catch (RuntimeException e) {
        evaluationStarted.set(false);
        throw e;
      }

Comment on lines +146 to +172
synchronized void startPeriodicEvaluation(GcpFallbackChannelOptions options) {
if (isShutdown
|| options == null
|| !options.isEnableFallback()
|| options.getPeriod() == null
|| options.getPeriod().toMillis() <= 0) {
return;
}
if (evaluationStarted.compareAndSet(false, true)) {
ScheduledExecutorService executor = getOrCreateExecutorService(options);
if (executor == null || executor.isShutdown()) {
evaluationStarted.set(false);
return;
}
GcpFallbackOpenTelemetry openTelemetry =
options.getGcpOpenTelemetry() != null
? options.getGcpOpenTelemetry()
: GcpFallbackOpenTelemetry.newBuilder().build();

scheduledEvaluationFuture =
executor.scheduleAtFixedRate(
() -> checkErrorRates(options, openTelemetry),
options.getPeriod().toMillis(),
options.getPeriod().toMillis(),
TimeUnit.MILLISECONDS);
}
}

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.

medium

When GcpFallbackState is shared across multiple GcpFallbackChannel instances, the periodic evaluation is started only once using the options of the first channel that initializes it (due to the evaluationStarted gate). If subsequent channels are configured with different options (e.g., different periods, thresholds, or telemetry setups), those configurations will be silently ignored for the shared evaluation loop.

Consider documenting this behavior clearly, or refactoring GcpFallbackState to accept its own configuration/options during construction so that shared configuration is explicit and consistent.

References
  1. In global static utilities shared across multiple concurrent sessions, avoid state changes triggered by a single session's configuration if those changes would negatively affect other active sessions.

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