Skip to content

refactor(spanner): decouple read-only transactions from async result sets - #14902

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-decouple-tx-from-async-result-set
Open

olavloite wants to merge 1 commit into
mainfrom
spanner-decouple-tx-from-async-result-set

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Decouple multi-use read-only transactions from internal async result set implementation details by introducing a dedicated lifecycle callback.

Key improvements in this change:

  • Replaces the anonymous subclass in read-only transactions with a clean callback that signals when an asynchronous query either begins execution or is closed without starting.
  • Lazily resolves the underlying query stream so that closing an async query before it begins execution no longer starts the query or opens an RPC stream, preventing unnecessary network calls and session allocations.
  • Permits safe inspection of query statistics and metadata after a result set has been closed.
  • Adds comprehensive unit tests for the decoupled lifecycle, post-close inspections, and closed-state error handling.

@olavloite
olavloite requested review from a team as code owners November 3, 2026 09:55

@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 AsyncResultSetImpl to introduce an onStartOrClose callback, allowing callers like MultiUseReadOnlyTransaction to track the query lifecycle more reliably. It simplifies the transaction logic by removing the anonymous class override and tracking state internally within AsyncResultSetImpl using synchronized monitor flags (startOrCloseNotified, delegateInitialized, delegateClosed). Additionally, it ensures that calling next() after closing the result set throws an IllegalStateException, and adds comprehensive unit tests to verify these behaviors. There are no review comments, and the implementation looks solid, so I have no feedback to provide.

@olavloite
olavloite force-pushed the spanner-decouple-tx-from-async-result-set branch from 62b05bd to aa1681b Compare November 3, 2026 15:01
@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 introduces an onStartOrClose callback to AsyncResultSetImpl to better track the query lifecycle, allowing callers like MultiUseReadOnlyTransaction to simplify pending start tracking. It also refactors delegate initialization and closing logic to prevent resource leaks and adds comprehensive unit tests. The review feedback highlights two important improvements: first, checking this.closed instead of delegateClosed in getDelegate() to prevent unnecessary RPC streams when closed immediately after setting a callback; second, wrapping the onStartOrClose callback execution in a try-catch block to avoid masking primary exceptions.

…sets

Decouple multi-use read-only transactions from internal async result set
implementation details by introducing a dedicated lifecycle callback.

Key improvements in this change:
- Replaces the anonymous subclass in read-only transactions with a clean
  callback that signals when an asynchronous query either begins execution
  or is closed without starting.
- Lazily resolves the underlying query stream so that closing an async query
  before it begins execution no longer starts the query or opens an RPC
  stream, preventing unnecessary network calls and session allocations.
- Permits safe inspection of query statistics and metadata after a result
  set has been closed.
- Adds comprehensive unit tests for the decoupled lifecycle, post-close
  inspections, and closed-state error handling.
@olavloite
olavloite force-pushed the spanner-decouple-tx-from-async-result-set branch from aa1681b to 7d470a9 Compare November 3, 2026 15:25
@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 AsyncResultSetImpl to introduce an onStartOrClose callback, allowing callers like MultiUseReadOnlyTransaction to track the query lifecycle more reliably. It simplifies the transaction logic by removing the custom anonymous subclass and replacing it with this callback. Additionally, the changes improve resource management, ensure proper delegate closing, add robust error handling for callback execution, and introduce comprehensive unit tests to verify these behaviors. No review comments were provided, so there is no feedback to address.

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