Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors AbstractReadContext.java by extracting several inner classes (SingleReadContext, SingleUseReadOnlyTransaction, and MultiUseReadOnlyTransaction) into separate top-level files, and consolidates streaming result set creation using a new createStreamingResultSet helper method. Feedback on these changes highlights a behavioral regression where the override of prepareIteratorForRetryOnDifferentGrpcChannel() was omitted in the refactored stream creation, which prevents channel rotation on retry. Additionally, it is recommended to avoid using the fully qualified class name for ExecuteSqlRequest.QueryMode since the class is already imported.
…am creation
This is a pure refactoring with zero behavioral or public API changes:
1. Subclass Extraction:
- Extract `SingleReadContext`, `SingleUseReadOnlyTransaction`, and
`MultiUseReadOnlyTransaction` from inner classes in `AbstractReadContext`
into dedicated package-private top-level classes in `com.google.cloud.spanner`.
- Add class-level Javadocs describing the lifecycle and purpose of each
read-only transaction type.
2. Stream Creation Deduplication:
- Deduplicate shared `ResumableStreamIterator` construction, stream listener
registration, transaction selector setup, and RPC execution error handling
between `executeQueryInternalWithOptions` and `readInternalWithOptions` in
`AbstractReadContext` using private helper methods (`createStreamingResultSet`,
`executeCall`, `prepareTransactionSelector`).
3. Import & Reference Cleanup:
- Remove obsolete inner-class imports across `SessionImpl` and `BatchClientImpl`.
- Remove redundant superclass static imports in the extracted classes.
- Update `AsyncReadOnlyTransactionTest` to reference the top-level
`MultiUseReadOnlyTransaction`.
7e21009 to
95c4343
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors AbstractReadContext by extracting several inner classes (SingleReadContext, SingleUseReadOnlyTransaction, and MultiUseReadOnlyTransaction) into their own top-level files. Additionally, it introduces a helper method createStreamingResultSet to consolidate streaming result set creation logic. A review comment identifies a regression in this helper method where the override for prepareIteratorForRetryOnDifferentGrpcChannel() was omitted, which prevents channel affinity retries for multiplexed sessions; a suggestion is provided to restore this override.
This is a pure refactoring with zero behavioral or public API changes:
SingleReadContext,SingleUseReadOnlyTransaction, andMultiUseReadOnlyTransactionfrom inner classes inAbstractReadContextinto dedicated package-private top-level classes incom.google.cloud.spanner.ResumableStreamIteratorconstruction, stream listener registration, transaction selector setup, and RPC execution error handling betweenexecuteQueryInternalWithOptionsandreadInternalWithOptionsinAbstractReadContextusing private helper methods (createStreamingResultSet,executeCall,prepareTransactionSelector).SessionImplandBatchClientImpl.AsyncReadOnlyTransactionTestto reference the top-levelMultiUseReadOnlyTransaction.