Code review comment for lp://staging/~free.ekanayaka/storm/tpc-support

Revision history for this message
Thomas Herve (therve) wrote :

[1]
+ global_transaction_id = getattr(txn, "__storm_transaction_id__", None)

Ideally, we would store this on an external object, but on something that we control instead. Maybe on the ZStorm instance, using a WeakKeyDictionary?

[2]
+ try:
+ self._store.commit()
+ except:
+ # We let the exception propagate, as the transaction manager
+ # will then call tcp_abort, and we will register the hook there
+ raise
+ else:
+ self._hook_register_transaction_event()

Looks equivalent to the following:

+ self._store.commit()
+ self._hook_register_transaction_event()

[3]
+ prepare() can't be used if a two-phase transaction has not began yet.

Typo: begun.

[4]
+ It's possible recover and commit pending transactions that were
+ prepared but not committed or rolled back.

+ It's possible recover and rollback pending transactions that were
+ prepared but not committed or rolled back.

Sounds confusing, it's missing a "to"?

It looks great! I'd be surprised if we get everything right in the first try, but the changes don't seem to impact the current behavior, which is nice. If possible, it'd be good to ask James H. to have a look, he probably have some interesting comments.

Thanks!

review: Approve

« Back to merge proposal