Tracking status of Transaction as well as success / failure.

[dhermes]

Fixes #496.

NOTE: Some of these changes may belong on Batch, but the concept
of "tombstone"-ing is unique to a Transaction (i.e. once started,
can only be committed once and the transaction ID can never be
used again).

@tseaver I initially tried to do this stuff in Batch.__enter__ and Batch.__exit__ but the re-use policies differ and I wanted this to work outside of context managers. LMK what you think.

[coveralls]

Coverage remained the same at 100.0% when pulling 63265b0 on dhermes:fix-496 into 9518db7 on GoogleCloudPlatform:master.

[tseaver]

(Originally mis-posted on #573):

I'm not sure about the use case: won't the user have already handled an exception in the case that the transaction commit failed, or else have aborted it directly?

On the implementation: handling two state fields seems awkward: there are nonsensical combinations of them (_status is None, but _commit_success is True, etc.). ISTM it would be better just to have self._status always be an integer, with values ranging from 0 ("initial"), 1 ("in progress"), 2 ("aborted"), 3 ("committed").

[dhermes]

OK, good call on that.

• Do you not think None is appropriate for "initial"?
• Do you think raising a ValueError in succeeded on "initial" or "in progress" is still appropriate?

[tseaver]

• I was thinking of status as an enum-like field.
• I'm not sure of the need for succeeded at all: calling code could just check the status directly (I still don't have a vision about what kind of code that would be, so that may be my problem).

[dhermes]

Well without this, there is no indication if the context manager exited with commit() or rollback() and there is no way to tell. People could have something like

with Transaction() as xact:
    datastore.put([entity])
    datastore.delete([key1, key2])
    foo()

and foo() could be raising, causing a rollback(), so it would be nice to check if xact.succeeded.

Doing sequential / dependent transactions would also rely on knowing that the previous commit succeeded (lot's of transient failures possible, like short-term 503 and 429 errors.)

[tseaver]

Application code would've needed to catch an exception, and would therefore know that the transaction aborted, no?

try:
    with Transaction() as xact:
        datastore.put([entity])
        datastore.delete([key1, key2])
        foo()
except SomeErrorType:
    recover_from_error()

Or else it would've been managing the transaction imperatively, and would have called commit() or rollback() directly:

xact = Transaction()
xact.put([entity])
xact.delete([key1, key2])
try:
    foo():
except SomeErrorType:
    xact.rollback()
else:
    xact.commit()

[dhermes]

I was targeting the context manager case, not direct interaction, due to the case you illustrate above.

I was also confused about context managers, thinking that they swallowed exceptions in all cases (only if __exit__ returns something Truth-y). Mind if I add a comment to the code explaining this?

Thus succeeded is not necessary. I'll axe it, but leave the rest of the code in and ping you when updated.

[dhermes]

@tseaver I rebased, removed succeeded, and switched to using 4 enum values. PTAL.

[coveralls]

Coverage remained the same at 100.0% when pulling 88cb162 on dhermes:fix-496 into 204389a on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same at 100.0% when pulling ed0762b on dhermes:fix-496 into 204389a on GoogleCloudPlatform:master.

[tseaver]

OK, one final question: do we want to expose a public, read-only status property (and therefore make the quasi-enum constants public too?).

[dhermes]

I don't see a reason to too at this time, and would rather not commit to it without a concrete use case.

With the private implementation, this PR really just serves to keep begin() from begin called twice.

[tseaver]

OK, LGTM