Repository navigation
fix(spanner): do not cache retryable errors in AbstractLazyInitializer - #14752
Open
sakthivelmanii wants to merge 2 commits into
Open
sakthivelmanii wants to merge 2 commits into
sakthivelmanii wants to merge 2 commits into
Conversation
When AbstractLazyInitializer.get() is called and initialize() fails with an exception, the failure was previously cached permanently regardless of whether the underlying error was transient or retryable. In MultiplexedSessionDatabaseClient and DatabaseClientImpl, this caused calls to getDialect() or getStatementFactory() to permanently cache transient startup or query errors (such as DEADLINE_EXCEEDED, RESOURCE_EXHAUSTED, or UNAVAILABLE) and continually rethrow the cached exception even after session creation succeeded on subsequent retries. This change modifies AbstractLazyInitializer to selectively cache only resource-not-found exceptions (DatabaseNotFoundException, InstanceNotFoundException, SessionNotFoundException) and non-retryable errors. Retryable SpannerExceptions (with error codes matching MultiplexedSessionDatabaseClient.RETRYABLE_ERROR_CODES: DEADLINE_EXCEEDED, RESOURCE_EXHAUSTED, UNAVAILABLE) are rethrown without setting initialized = true, allowing subsequent get() calls to retry initialization. In addition: - Relaxed visibility of MultiplexedSessionDatabaseClient.RETRYABLE_ERROR_CODES from private to package-private to share the retryable error classification. - In DatabaseClientImpl.statementFactorySupplier, unwrapped ExecutionException to surface the underlying cause to SpannerExceptionFactory.asSpannerException, and converted TimeoutException via SpannerExceptionFactory.propagateTimeout to produce DEADLINE_EXCEEDED. - Added comprehensive unit tests in LazySpannerInitializerTest and DatabaseClientImplTest verifying retry behavior on transient errors and concurrent retry handling.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates AbstractLazyInitializer to allow retrying initialization when transient, retryable errors occur, rather than caching them indefinitely. It also refactors exception handling in DatabaseClientImpl when retrieving the database dialect and exposes RETRYABLE_ERROR_CODES from MultiplexedSessionDatabaseClient. Comprehensive tests have been added to verify recovery behavior. The review feedback suggests simplifying the shouldCacheError method in AbstractLazyInitializer by removing redundant checks for specific exception subclasses, as they are already covered by the more general SpannerException check.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
AbstractLazyInitializer.get()is invoked andinitialize()throws an exception, the failure was previously permanently cached inthis.errorwiththis.initialized = trueregardless of whether the error was transient or retryable. InMultiplexedSessionDatabaseClient.getDialect()andDatabaseClientImpl.getStatementFactory(), an initial transient failure during session creation or query execution (DEADLINE_EXCEEDED,RESOURCE_EXHAUSTED, orUNAVAILABLE) caused all subsequent calls to immediately rethrow the cached exception without retrying, even after multiplexed session creation succeeded on retry.Solution
Update
AbstractLazyInitializerto cache only resource-not-found exceptions (DatabaseNotFoundException,InstanceNotFoundException,SessionNotFoundException) and non-retryable errors. RetryableSpannerExceptionerrors (matchingMultiplexedSessionDatabaseClient.RETRYABLE_ERROR_CODES:DEADLINE_EXCEEDED,RESOURCE_EXHAUSTED,UNAVAILABLE) are rethrown without settinginitialized = trueso subsequentget()calls retry initialization.MultiplexedSessionDatabaseClient.RETRYABLE_ERROR_CODESpackage-private soAbstractLazyInitializershares the same retryable error set.DatabaseClientImpl.statementFactorySupplier.initialize(), unwrapExecutionException.getCause()inSpannerExceptionFactory.asSpannerExceptionand convertTimeoutExceptionviaSpannerExceptionFactory.propagateTimeout(DEADLINE_EXCEEDED).LazySpannerInitializerTestandDatabaseClientImplTestcovering retryable error recovery, non-retryable error caching, and concurrent retries.