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 5 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.
There was a problem hiding this comment.
After creating an inline-begin selector, both read and query start paths wrap
the RPC start in a try/catch and call onStartFailed(withBeginTransaction, t), which completes transactionIdFuture exceptionally. onError() and onDone() also fail the future if the stream starts but no transaction id is returned.
| * <p>Options can include: | ||
| * | ||
| * <ul> | ||
| * <li>{@link Options#beginTransactionOption(Options.BeginTransactionOption)}: Controls whether |
There was a problem hiding this comment.
This part of the documentation should also be added to the method above that does not accept a TimestampBound. Also, I think it would be good to include some arguments for when it makes sense to use this option, and when it does not make sense.
|
|
||
| try { | ||
| return TransactionSelector.newBuilder() | ||
| .setId(futureToWaitFor.get(WAIT_FOR_INLINE_BEGIN_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)) |
There was a problem hiding this comment.
This means that when using inline-begin-tx and you run multiple queries concurrently at the start of the transaction, the queries that do not include the BeginTransaction option effectively get a different timeout behavior than today; if the query that starts the transaction is slow, then the other queries could fail. It might be good to include this in the documentation for this option (or maybe more in general for the documentation: Basically advice against using this option when the read-only transaction executes queries in parallel)
| Timestamp readTimestamp = null; | ||
| if (transaction.hasReadTimestamp()) { | ||
| try { | ||
| readTimestamp = Timestamp.fromProto(transaction.getReadTimestamp()); |
There was a problem hiding this comment.
Note that this change does introduce a behavioral difference between inline-begin and explicit-begin, albeit one that is perfectly valid. With explicit-begin, you can call getReadTimestamp() right away starting the transaction. With inline-begin, the getReadTimestamp() method will throw an error if you try to call it before at least one PartialResultSet has been returned by the first query. This is an additional reason why we cannot make this behavior the default without a major version bump (or we would need to change this behavior into something that would be safe).
There was a problem hiding this comment.
Thanks, updated the comment
| @Override | ||
| public ReadOnlyTransaction readOnlyTransaction(TimestampBound bound) { | ||
| return readOnlyTransaction( | ||
| bound, Options.beginTransactionOption(Options.BeginTransactionOption.EXPLICIT)); |
There was a problem hiding this comment.
Would it not make more sense to pass in an empty array of read-only transaction options here? By passing in an explicit value here, you are essentially overriding any future changes to the default that we want to use, and/or if we for example want to add a field to SpannerOptions for setting a client-wide default.
| } | ||
|
|
||
| @Test | ||
| public void multiUseReadOnlyTransactionCanUseInlineBegin() throws ParseException { |
There was a problem hiding this comment.
This is the only test that is added for this entire feature. That is not enough. We should add tests that verify:
- The happy path (does inline-begin actually do anything?)
- Unhappy-paths and other non-standard cases: What if the first query fails? What if there are multiple concurrent queries at start? Does this also work for Read operations? Does this work for our async API? What happens if an application calls executeQuery(..) and then closes the ResultSet without ever reading anything?
2 participants
