feat(grpc-gcp): Add shared fallback state and probing recovery options to GcpFallbackChannel - #14013
kinsaurralde wants to merge 18 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.
c44f3b5 to
a183b5c
Compare
a183b5c to
bfce1d5
Compare
|
/gemini review |
There was a problem hiding this comment.
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.
| scheduledEvaluationFuture = | ||
| executor.scheduleAtFixedRate( | ||
| () -> checkErrorRates(options, openTelemetry), | ||
| options.getPeriod().toMillis(), | ||
| options.getPeriod().toMillis(), | ||
| TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
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;
}| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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
- 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.
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.