storage: untangle aborting transactions from cancelling batches

[tamird]

Currently, whenever an error is returned from EndTransaction, the "batch" (engine.Engine) is never committed. In particular, any local intent resolution is discarded. This is problematic in the case of wanting to abort a transaction and resolve its intents because the abortion is signalled with TransactionAbortedError, which is an error, as its name suggests, and prevents the local intent resolution from being committed.

We either need to signal transaction abortion separately from errors, or else introduce special casing for certain errors so that they do not prevent engine.Engine "batches" from committing.

This came up in #3028.

[tbg]

from the discussion there:

you're returning an error, and that means that whatever you've written to the batch is actually not executed, in particular the resolution of your local intents is tossed. I should've remembered that this was an issue (notice that the other TransactionAbortedError above doesn't resolve its local intents here but passes them to the caller, for precisely that reason). There are some options:

a) you return all intents here, not only externalIntents, but you need to make sure that they all have the transaction as ABORTED. That way they're going to remove the intents, but you have no way of actually writing ABORTED in the master proto. That's super dirty and there might be an anomaly hidden somewhere.
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).
c) we break with the invariant that a command that errors out can't write to the engine. See #1989

I personally think the prettiest option is dropping TransactionAbortedError altogether, though it's late on a Friday and I might be missing something.

[tamird]

Did #5885 really resolve this?

On Thu, Apr 7, 2016 at 11:26 AM, Kenji Kaneda notifications@github.com
wrote:

Closed #3037 #3037 via
#5885 #5885.

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#3037 (comment)

[tbg]

As of recently, we already special-case *TransactionRetryError (and still commit the batch if it occurs). I imagine that opens the door to special-casing some more if we need to (or putting a less ad-hoc mechanism in place), so keeping this issue open doesn't add much.

[kkaneda]

ah, sorry.. wasn't intentional.