feat: Add configurable retry timing for RunTransactionAsync #11564
Conversation
jskeet
added
the
do not merge
|
Marked as "do not merge" as while this is the complete production code as far as I'm aware, there are no tests yet - I wanted to get feedback on the API and overall plan first. |
| @@ -410,8 +420,12 @@ public async Task<T> RunTransactionAsync<T>(Func<Transaction, Task<T>> callback, | |||
| } | |||
| catch (RpcException e) when (CheckRetry(e, ref rollback)) | |||
There was a problem hiding this comment.
I think there's a bug here that was there before.
If commit fails with aborted and there are attempts left, we never rollback that failed transaction right? Or is it OK not to rollback it because it already failed?
There was a problem hiding this comment.
I think it's okay, but we'll need to check. We definitely need to make changes here in general though - this PR is only attempting to affect the timing.
| // This is essentially the inner loop of RetryAttempt.CreateRetrySequence. | ||
| await scheduler.Delay(timing.BackoffJitter.GetDelay(backoff), cancellationToken).ConfigureAwait(false); | ||
| backoff = timing.NextBackoff(backoff); |
There was a problem hiding this comment.
I think this is clearer inside the catch.
There was a problem hiding this comment.
I'm generally nervous of doing stuff in catch blocks. There are some oddities about how code executes there - not that I can remember details. Let's chat next week.
| @@ -24,27 +25,53 @@ public sealed class TransactionOptions | |||
| /// <summary> | |||
| /// The transaction options that are used if nothing is specified by the caller. | |||
| /// </summary> | |||
| public static TransactionOptions Default { get; } = new TransactionOptions(5); | |||
| public static TransactionOptions Default { get; } = new TransactionOptions(5, null); | |||
|
|
|||
| /// <summary> | |||
| /// The number of times the transaction will be attempted before failing. | |||
| /// </summary> | |||
| public int MaxAttempts { get; } | |||
There was a problem hiding this comment.
Could we deprecate this one instead of ignoring the one in RetrySettings?
There was a problem hiding this comment.
Feels a bit like overkill if most people won't need to touch the timing - creating a retry setting is relatively complex. Happy to chat about it though.
|
Revamped to just use an underlying RetrySettings - PTAL. |
There are further changes to come in terms of transaction retry, but this is at least a start. Note that this changes the default backoff from "none at all" to "100ms initial, with a multiplier of 1.3". That's a more reasonable default, and it seems unlikely that customers would actually *depend* on there being no backoff. Fixes googleapis#10513
jskeet
removed
the
do not merge
|
I've added tests, and this is now ready to merge if you're happy with it. (Again, it's not the final word on transaction retries - but it's at least an improvement in terms of timing.) |
There are further changes to come in terms of transaction retry, but this is at least a start. Note that this changes the default backoff from "none at all" to "100ms initial, with a multiplier of 1.3". That's a more reasonable default, and it seems unlikely that customers would actually depend on there being no backoff.
Fixes #10513