Visitar URL original
fix(bigtable): avoid background retry amplification and log failed config polls by sushanb · Pull Request #14980 · googleapis/google-cloud-java · GitHub
Skip to content

fix(bigtable): avoid background retry amplification and log failed config polls - #14980

Open
sushanb wants to merge 1 commit into
googleapis:mainfrom
sushanb:fix/bigtable-config-manager-background-poll-retries
Open

sushanb wants to merge 1 commit into
googleapis:mainfrom
sushanb:fix/bigtable-config-manager-background-poll-retries

Conversation

@sushanb

@sushanb sushanb commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Problem

  1. Silent background poll failures:
    Background polls launched from scheduleNextPoll() did not log Throwable causes or record the client_config_fetch_failed debug tag when handleFailedFetch returned false, leaving operators with no log or telemetry visibility into recurring background GetClientConfiguration failures.

  2. Background retry & channel-rebuild amplification (max_rpc_retry_count = 5):
    When validUntil expired after failed polls (or after a failed initial start()), clientConfiguration fell back to defaultConfig, which carries the startup retry budget max_rpc_retry_count = 5. Every subsequent background poll inherited max_rpc_retry_count = 5 and executed 6 RPC attempts and 5 replaceChannel() rebuilds per poll interval instead of a single background poll attempt.

Fix

  • Preserve the defaultConfig (session_load = 0) fallback when validUntil expires, while keeping defaultConfig's max_rpc_retry_count = 5 scoped to the initial blocking start() poll rather than background polls when usingDefaultConfig is true.
  • Log terminal background poll failures and validUntil expiry resets at Level.WARNING with the underlying Throwable, log intermediate retry attempts at Level.FINE, and record the client_config_fetch_failed debug tag on terminal background poll failures.

Testing

  • Added expiredConfigFallsBackToDefaultWithoutRetryAmplificationAndLogsFailure verifying pre-expiry failure handling, post-expiry defaultConfig (session_load = 0) fallback, absence of 6x retry / replaceChannel() amplification on background polls, WARNING log + DebugTagTracer emission, and subsequent recovery.
  • Added failedInitialFetchDoesNotAmplifySubsequentBackgroundPollRetries verifying start() retains its 5-retry startup budget while subsequent background polls make a single attempt without channel-rebuild amplification.

@sushanb
sushanb requested review from a team as code owners November 12, 2026 07:24

@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 refactors ClientConfigurationManager to prevent retry amplification during background configuration polls, particularly when falling back to the default configuration. It introduces a Clock dependency for testability, tracks whether the default configuration is active, and adds comprehensive tests using a mutable clock. The review feedback highlights a bug where validUntil is not reset when falling back to a configuration without polling capabilities (such as defaultConfig). This causes subsequent poll failures to repeatedly trigger expired validity resets and log misleading warnings. A code suggestion is provided to reset validUntil to Instant.MAX in this case.

Comment on lines 551 to 557
if (clientConfiguration.hasPollingConfiguration()) {
this.validUntil =
Instant.now()
clock
.instant()
.plus(
toJavaDuration(
clientConfiguration.getPollingConfiguration().getValidityDuration()));

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 falling back to defaultConfig (or any configuration without a polling configuration), validUntil is not updated and remains at its expired value (which is in the past). Consequently, every subsequent background poll failure will repeatedly trigger shouldReset = true, logging the misleading warning "Client config validity expired after failed poll; falling back to default config" and repeatedly setting the configuration, instead of logging "Failed to refresh client config".

To fix this, reset this.validUntil to Instant.MAX when the configuration does not have a polling configuration.

Suggested change
if (clientConfiguration.hasPollingConfiguration()) {
this.validUntil =
Instant.now()
clock
.instant()
.plus(
toJavaDuration(
clientConfiguration.getPollingConfiguration().getValidityDuration()));
if (clientConfiguration.hasPollingConfiguration()) {
this.validUntil =
clock
.instant()
.plus(
toJavaDuration(
clientConfiguration.getPollingConfiguration().getValidityDuration()));
} else {
this.validUntil = Instant.MAX;
}

This branch has not been deployed

No deployments
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