Skip to content

refactor(spanner): decompose AbstractReadContext and deduplicate stream creation - #14667

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-refactor-abstract-read-context
Open

olavloite wants to merge 1 commit into
mainfrom
spanner-refactor-abstract-read-context

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

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.

@olavloite
olavloite requested review from a team as code owners October 16, 2026 15:19

@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 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`.
@olavloite
olavloite force-pushed the spanner-refactor-abstract-read-context branch from 7e21009 to 95c4343 Compare October 16, 2026 15:33
@olavloite

Copy link
Copy Markdown
Contributor Author

/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 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 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