Spanner: Use generic retry helper for transaction retries #5433
Conversation
googlebot
added
the
cla: yes
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| throw e; | ||
| } | ||
| } | ||
| private <T> T runInternal(final TransactionCallable<T> txCallable) { |
There was a problem hiding this comment.
Would you mind just briefly explaining what this change is? Hard to see from the diff. Is it mainly the return statement at the bottom?
There was a problem hiding this comment.
The web diff view is indeed not very helpful in this case...
In general: Instead of using a while(true) {...} loop for retrying the transaction, the transaction is placed in a Callable that can be passed to the generic retry helper. The retry helper will handle the retries if the transaction aborts.
More specifically, the change is as follows:
- Everything inside the
while(true)loop is moved into thecall()method of theCallableretryCallable. - The
session.newTransaction()call is moved from the constructor and thebackoff(...)method to inside thecall()method to give each attempt a fresh transaction. - The
continuestatement on line 354 is replaced with athrowstatement in order to leave the method exceptionally. - The
Callableis passed in to the new methodSpannerRetryHelper.runTxWithRetriesOnAborted(Callable)that handles the retries if the transaction is aborted.
| while (true) { | ||
| checkState( | ||
| isValid, "TransactionRunner has been invalidated by a new operation on the session"); | ||
| SpannerImpl.checkContext(context); |
There was a problem hiding this comment.
What happens if the context is cancelled now? Are the same error messages thrown for deadline exceeded and cancelled?
There was a problem hiding this comment.
In the initial version of this PR, the context was ignored. I've added a check for context cancellation to the generic retry helper, and it will now throw the same exceptions as previous retry loops.
Codecov Report
@@ Coverage Diff @@
## master #5433 +/- ##
============================================
+ Coverage 50.5% 51.09% +0.58%
- Complexity 23903 24475 +572
============================================
Files 2259 2271 +12
Lines 227870 232042 +4172
Branches 24994 25064 +70
============================================
+ Hits 115088 118552 +3464
- Misses 104163 104838 +675
- Partials 8619 8652 +33
Continue to review full report at Codecov.
|
kolea2
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| class SpannerRetryHelper { | ||
| private static final RetrySettings txRetrySettings = | ||
| RetrySettings.newBuilder() | ||
| .setInitialRetryDelay(Duration.ofMillis(1000L)) |
There was a problem hiding this comment.
Minor - I would take these numbers and make them constants.
A Spanner transaction runner that was given a session with a prepared read/write transaction would not use that transaction, but instead always start a new one, causing an extra unnecessary round-trip to the server. This regression was introduced by commit d305770 (PR googleapis#5433).
5 participants
As discussed in PR #5412 we should move away from custom retry loops and use a generic retry helper. This PR changes the retry loop of read/write transactions from a custom loop to using the Gax
RetryHelper.