Refactor PessimisticTransaction #2691
Conversation
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
maysamyabandeh
force-pushed
the
txn-template-txnimpl2
branch
from
5463502
to
94fbc09
Compare
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -111,6 +110,12 @@ class PessimisticTxn : public TransactionBaseImpl { | |||
| int64_t GetDeadlockDetectDepth() const { return deadlock_detect_depth_; } | |||
|
|
|||
| protected: | |||
| virtual Status PrepareInternal() = 0; | |||
|
|
|||
| virtual Status CommitSingleInternal() = 0; | |||
There was a problem hiding this comment.
What does commit_single mean? Seems it means a non-2PC transaction, without any consistency checking? Add comment here to explain.
And if the transaction API allow someone call Commit() without Prepare(), what if someone mean to use 2PC, but forget to call Prepare()..
There was a problem hiding this comment.
commit_single is inherited from the existing code. Admitted that it is not a descriptive name. Let me change it to commit_without_prepare and comment to clarify that.
That is a good point. It makes sense to reject commit if 2pc is set. To do that properly we would need to ask users to explicitly set/unset an option in each transaction if they mean to bypass prepare for that. Let us keep that in mind as a future work.
maysamyabandeh
force-pushed
the
txn-template-txnimpl2
branch
from
17900f7
to
463dddd
Compare
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
maysamyabandeh
changed the title
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This patch splits Commit and Prepare into lock-related logic and db-write-related logic. It moves lock-related logic to PessimisticTransaction to be reused by all children classes and movies the existing impl of db-write-related to PrepareInternal, CommitSingleInternal, and CommitInternal in WriteCommittedTxnImpl.