Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new package-private abstraction ReadOrQueryRequest (with Query and Read subclasses) to/unify common request options, streaming request preparation, and execution within AbstractReadContext. It refactors stream-start error handling and transaction selector management, particularly for inlined BeginTransaction options, ensuring that failures during initial statements are correctly propagated and handled without hanging. Additionally, it updates the GrpcStreamIterator and TransactionRunnerImpl classes to support these changes and adds extensive unit tests covering stream retries, start failures, and concurrency scenarios. There are no review comments to address, and I have no feedback to provide.
Streaming SQL queries and streaming key-value reads previously duplicated the same request setup and stream startup logic in two separate code paths. This change unifies both operations behind a single internal request abstraction so that option configuration, retry settings, and stream lifecycle handling are maintained in one place. As part of unifying stream startup, this also hardens how inlined BeginTransaction state is tracked across the stream lifecycle: - Records whether a request carries an inlined BeginTransaction on the request itself and passes it to the stream iterator at construction time rather than when the gRPC call is attached, keeping the state available across retry backoffs while ensuring it is cleared once the iterator is closed. - Ensures that if starting an inlined-begin stream fails before the stream can be consumed, the pending transaction ID future is failed immediately with a consistent exception so concurrent callers on the same transaction do not wait for a timeout.
8249a04 to
393f3b2
Compare
| ExecuteSqlRequest.newBuilder() | ||
| .setSql(statement.getSql()) | ||
| .setQueryMode(queryMode) | ||
| .setSession(session.getName()); |
There was a problem hiding this comment.
All options that are common for Read and Query have been moved into ReadOrQueryRequest, and are therefore removed here to reduce code duplication.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package-private abstraction, ReadOrQueryRequest, to unify the preparation and execution of read and query requests within AbstractReadContext. It refactors the streaming result set creation to utilize this unified request model, simplifying the codebase and improving consistency. Additionally, error handling and propagation for inlined begin transactions are enhanced in TransactionRunnerImpl and GrpcStreamIterator, backed by comprehensive new test cases. Feedback on the changes suggests a minor improvement in TransactionRunnerImpl.java to check exception.getErrorCode() == ErrorCode.ABORTED instead of using instanceof AbortedException for better robustness and consistency with the rest of the SDK.
Streaming SQL queries and streaming key-value reads previously duplicated the same request setup and stream startup logic in two separate code paths. This change unifies both operations behind a single internal request abstraction so that option configuration, retry settings, and stream lifecycle handling are maintained in one place.
As part of unifying stream startup, this also hardens how inlined BeginTransaction state is tracked across the stream lifecycle: