Do not return TransactionAbortedError when rolling back an aborted txn

[kkaneda]

(I was curious if we can fix #3037 (and #1989). The change introduces some complexity and I'm not totally sure we should fix these issues.)

This commit changes EndTransaction to return nil error on txn abort so that the we can commit a batch and update the transaction record.

Change txnSender.Send so that it convert a nil error to TransactionAbortedError so that txn.Exec will initiate a restart. This change is hacky, but I couldn't come up with a better approach.

We still return TransactionAbortedError in other places and keep the code path for handling TransactionAbortedError.

---

This change is

[tbg]

I like the idea of letting an EndTransaction{Commit:false} succeed even if it finds an already aborted transaction. Assuming there are no weird caveats, this can help "bring the intents home". Plus, we potentially double-abort transactions in various places, and making this not error out can allow us to start logging errors which were previously too noisy. I think that part of the change should be safe unless there's code out there which derives from a successful abort the information that the transaction was previously alive.

On the other hand, your change seems to go further and is half-way to getting rid of TransactionAbortedError, which I don't think is a change we want to make at the moment - changing semantics in that way is brittle. What's the reason you made those changes?

---

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

storage/replica_test.go, line 1771 [r1] (raw file):
s/Error/Fatal/

---

storage/replica_test.go, line 2097 [r1] (raw file):
if test.expErrRegexp == "

---

Comments from Reviewable

[tamird]

Not getting an error in the deadline exceeded case seems particularly problematic, since the caller (or grand-caller) of the transaction is unlikely to inspect the txn proto (or even have access to it) after EndTransaction.

---

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

client/txn.go, line 83 [r1] (raw file):
use GetArg?

---

Comments from Reviewable

[kkaneda]

Ah, I see. The idea was briefly mentioned in #3037:

b) if the commit only fails because of the deadline, you return this information not as an error but encoded in the response. This is similar to the recent discussions about ConditionalPut. A client would not receive a TransactionAbortedError but instead find out that its proto isn't PENDING any more. Semantically that should actually work out well, but needs a bunch of refactoring (TransactionAbortedError would just go away, we don't really need it - a BatchResponse carries a transaction proto as well).

If I understand correctly, this allows us to commit a batch and update a transaction record. We can instead tweak applyRaftCommandInBatch to write on TransactionAbortedError like TransactionRetryError, but I thought just dropping TransactionAbortedError would be simpler (and I had fewer test failures with that approach than the other approach).

I thought a caller can still inspect an error since we convert a nil-error to NewTransactionAbortedError in txnSender.Send.

Having said, I totally agree that the change is (very) brittle. Let me try getting rid of the part and see if just making the change to the rollback part works. Thanks!

[tamird]

I thought a caller can still inspect an error since we convert a nil-error to NewTransactionAbortedError in txnSender.Send.

Ah, I missed that - that seems right. We just need to adjust the tests so we can demonstrate how a user of a transaction with a deadline would actually see an error (currently the tests are changed to inspect the proto).

[tbg]

I'm against changing the deadline semantics at this point. Let's keep it simple and swallow errors only if the intention was to abort and there is indeed a transaction aborted proto. That way the only change in semantic is that you don't fail to abort if someone else did it for you already. If you're attempting to commit with a deadline constraint, I think you should get the error - what you wanted to happen did not happen.

[kkaneda]

Sure, updated the PR. It is now simple and just changes Replica.EndTransation to return a nil-error when it finds a transaction that has already been aborted, but the request is requesting a rollback (Commit = false). Could you take another look?

[tbg]

---

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

storage/replica_command.go, line 371 [r2] (raw file):
Add to the comment that rolling back an already rolled-back txn is ok.

---

storage/replica_command.go, line 372 [r2] (raw file):
Could you remove that TODO?

---

storage/replica_command.go, line 416 [r2] (raw file):
add to the TODO:

Add external intents to txn proto?

---

storage/replica_test.go, line 2128 [r2] (raw file):
From here on it's like above, so you could for i := range []int{0,1} { ... }.

---

Comments from Reviewable

[kkaneda]

Thanks for the review!

---

Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions.

---

storage/replica_command.go, line 371 [r2] (raw file):
Done.

---

storage/replica_command.go, line 372 [r2] (raw file):
Done.

---

storage/replica_command.go, line 416 [r2] (raw file):
Done.

---

storage/replica_test.go, line 2128 [r2] (raw file):
Done.

---

Comments from Reviewable