feat(spanner): add option for inline begin with multi-use read only txn#13233
feat(spanner): add option for inline begin with multi-use read only txn#13233rahul2393 wants to merge 2 commits into
Conversation
rahul2393
commented
May 20, 2026
rahul2393
commented
- Add optional inline begin support for multi-use read-only transactions.
- Preserve explicit BeginTransaction as default behavior.
Probers running multi read with 2 read at fixed QPS of 10 show improvement of around 600-700us using DirectPath |
![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 enables inlining the BeginTransaction RPC within the first operation of a multi-use read-only transaction to reduce latency. Feedback highlighted a compilation error and logic issue in the transaction state check, potential future leaks during RPC initialization, and thread starvation risks from blocking calls. The reviewer also advised on proper InterruptedException handling and the use of more specific error codes for client-side timeouts.
| if (shouldUseInlinedBegin()) { | ||
| SessionImpl.throwIfTransactionsPending(); | ||
| } else { | ||
| initTransaction(); | ||
| } |
There was a problem hiding this comment.
The call to SessionImpl.throwIfTransactionsPending() is problematic for several reasons:
- Correctness:
throwIfTransactionsPending()is an instance method inSessionImpl. Calling it as a static method will result in a compilation error. - Logic Error: Even if called on the
sessioninstance, this check will likely throw anIllegalStateExceptionbecause the currentMultiUseReadOnlyTransactionis already set as the active transaction on the session. - Redundancy: The check for pending transactions is already performed in
SessionImpl.setActive()when the transaction is initialized.
For inlined begin, we should simply skip initTransaction() without adding this extra check.
if (!shouldUseInlinedBegin()) {
initTransaction();
}| if (transactionIdFuture == null) { | ||
| transactionIdFuture = SettableApiFuture.create(); | ||
| return TransactionSelector.newBuilder() | ||
| .setBegin(createReadOnlyTransactionOptions()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
There is a potential for transactionIdFuture to be leaked (never completed) if the thread that returns the begin selector fails after creating the future but before the RPC is successfully initiated. In readInternal or executeQueryInternal, if an exception is thrown after getTransactionSelector() returns but before the RPC consumer is registered, onError or onDone will never be called. Consider adding a mechanism to ensure the future is failed if the 'begin' operation fails to start.
1 participant
