feat: support begin with AbortedException for manager interface #3835
Conversation
product-auto-label
bot
added
size: m
product-auto-label
bot
added
size: l
harshachinta
changed the title
| @@ -793,6 +793,7 @@ public SpannerException onError(SpannerException e, boolean withBeginTransaction | |||
| long delay = -1L; | |||
| if (exceptionToThrow instanceof AbortedException) { | |||
| delay = exceptionToThrow.getRetryDelayInMillis(); | |||
| ((AbortedException) exceptionToThrow).setTransactionID(this.transactionId); | |||
There was a problem hiding this comment.
Should this be
this.transactionId or
this.transactionId != null ? this.transactionId : this.getPreviousTransactionId()
There was a problem hiding this comment.
I guess this.transactionId != null ? this.transactionId : this.getPreviousTransactionId() would cause the transaction priority to be preserved also when the begin call of a transaction fails, right? In that case, I think we should do that.
| * Initializes a new read-write transaction. This method must be called before performing any | ||
| * operations, and it can only be invoked once per transaction lifecycle. |
There was a problem hiding this comment.
| * Initializes a new read-write transaction. This method must be called before performing any | |
| * operations, and it can only be invoked once per transaction lifecycle. | |
| * Initializes a new read-write transaction that is a retry of a previously aborted transaction. | |
| * This method must be called before performing any | |
| * operations, and it can only be invoked once per transaction lifecycle. |
| * <p>This is especially useful in scenarios involving multiplexed sessions and when creating a | ||
| * new transaction for retry attempts. If {@link #resetForRetryAsync()} is not used, you can pass | ||
| * the {@link AbortedException} from a previous attempt here to preserve the transaction's | ||
| * priority. |
There was a problem hiding this comment.
| * <p>This is especially useful in scenarios involving multiplexed sessions and when creating a | |
| * new transaction for retry attempts. If {@link #resetForRetryAsync()} is not used, you can pass | |
| * the {@link AbortedException} from a previous attempt here to preserve the transaction's | |
| * priority. | |
| * <p>This method should only be used when multiplexed sessions are enabled to create a retry | |
| * for a previously aborted transaction. This method can be used instead of {@link #resetForRetryAsync()} | |
| * to create a retry. Using this method or {@link #resetForRetryAsync()} will have the same effect. | |
| * You must pass in the {@link AbortedException} from the previous attempt to preserve the transaction's | |
| * priority. |
| private ApiFuture<TransactionContext> internalBeginAsync(boolean firstAttempt) { | ||
| @Override | ||
| public TransactionContextFutureImpl beginAsync(AbortedException exception) { | ||
| Preconditions.checkState(txn == null, "begin can only be called once"); |
There was a problem hiding this comment.
| Preconditions.checkState(txn == null, "begin can only be called once"); | |
| Preconditions.checkState(txn == null, "begin can only be called once"); | |
| Preconditions.checkNotNull(exception, "AbortedException from the previous attempt is required"); |
| @@ -61,6 +61,18 @@ enum TransactionState { | |||
| */ | |||
| TransactionContext begin(); | |||
|
|
|||
| /** | |||
| * Initializes a new read-write transaction. This method must be called before performing any | |||
There was a problem hiding this comment.
See my comment on beginAsync. I think that we should be more explicit in when this method should be used, and not describe it as 'useful in scenarios involving multiplexed sessions'. I think that could lead to wrong usage of this method.
| @Override | ||
| public TransactionContext begin(AbortedException exception) { | ||
| Preconditions.checkState(txn == null, "begin can only be called once"); | ||
| ByteString previousAbortedTransactionID = |
There was a problem hiding this comment.
Here also: Add a not-null check for the AbortedException.
| try (IScope s = tracer.withSpan(span)) { | ||
| txn = session.newTransaction(options, /* previousTransactionId = */ ByteString.EMPTY); | ||
| txn = session.newTransaction(options, /* previousTransactionId = */ previousTransactionId); |
There was a problem hiding this comment.
nit: you can remove the comment for previousTransactionId, as that is now clear from the variable name
| @@ -793,6 +793,7 @@ public SpannerException onError(SpannerException e, boolean withBeginTransaction | |||
| long delay = -1L; | |||
| if (exceptionToThrow instanceof AbortedException) { | |||
| delay = exceptionToThrow.getRetryDelayInMillis(); | |||
| ((AbortedException) exceptionToThrow).setTransactionID(this.transactionId); | |||
There was a problem hiding this comment.
I guess this.transactionId != null ? this.transactionId : this.getPreviousTransactionId() would cause the transaction priority to be preserved also when the begin call of a transaction fails, right? In that case, I think we should do that.
|
Warning: This pull request is touching the following templated files:
|
Aborted transactions should ideally be retried in the same transaction manager instance because the client library will ensure to populate the "Prev txn attempt's txn id" on retry (for mux sessions) which helps preserve the lock order(priority) of the transaction in scenarios of high contention thats causing txn wounding. This is achieved by using
resetForRetry()that automatically takes care of preserving the lock order.But if the customer application retries aborted transactions on a new TransactionManager instance with out using the
resetForRetry(), we lose the lock order of the previous attempt.To address this scenario of preserving the lockorder across transaction manger instance for a retry of the same transaction
Current proposal as per client lib team recommendation:
Note: This will be a no-op for regular sessions.
b/407037309