Refactor TransactionDBImpl #2689
Conversation
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -122,5 +123,39 @@ class TransactionDBImpl : public TransactionDB { | |||
| std::unordered_map<TransactionName, Transaction*> transactions_; | |||
| }; | |||
|
|
|||
| class WriteCommittedTxnDBImpl : public PessimisticTxnDB { | |||
There was a problem hiding this comment.
I am not sure why do we say WriteCommittedTxnDBImpl instead of WriteCommittedTxnDB
I guess we should not if there is not a WriteCommittedTxnDB
There was a problem hiding this comment.
Makes sense. I will drop "Impl"
| @@ -122,5 +123,39 @@ class TransactionDBImpl : public TransactionDB { | |||
| std::unordered_map<TransactionName, Transaction*> transactions_; | |||
| }; | |||
|
|
|||
| class WriteCommittedTxnDBImpl : public PessimisticTxnDB { | |||
There was a problem hiding this comment.
I also think these two classes need some comments
| @@ -21,8 +21,8 @@ | |||
|
|
|||
| namespace rocksdb { | |||
|
|
|||
| TransactionDBImpl::TransactionDBImpl(DB* db, | |||
| const TransactionDBOptions& txn_db_options) | |||
| PessimisticTxnDB::PessimisticTxnDB(DB* db, | |||
There was a problem hiding this comment.
I think we should rename the files if we rename the classes
| Transaction* old_txn) override; | ||
| }; | ||
|
|
||
| class WritePreparedTxnDBImpl : public PessimisticTxnDB { |
| @@ -27,7 +27,7 @@ struct LockMap; | |||
| struct LockMapStripe; | |||
|
|
|||
| class Slice; | |||
| class TransactionDBImpl; | |||
| class PessimisticTxnDB; | |||
There was a problem hiding this comment.
I guess the decision to use PessimisticTxn was already taken in a different PR, but I would suggest to be consistent with the existing name which is OptimisticTransaction
so I would suggest using PessimisticTransaction and PessimisticTransactionDB
There was a problem hiding this comment.
Transaction makes the name child classes too long. I can change PessimisticTransaction to PessimisticTxn though.
There was a problem hiding this comment.
Even though I like the longer name more :D I am fine with both names as long as we are consistent
There was a problem hiding this comment.
ah it became difficult. OptimisticTransactionDB is in the include so we cannot rename it.
How about renaming PessimisticTxn to PessimisticTransaction but keep the short Txn for its children?
There was a problem hiding this comment.
ok I applied the last suggestion.
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@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. |
| @@ -115,7 +115,7 @@ TransactionLockMgr::TransactionLockMgr( | |||
| mutex_factory_(mutex_factory) { | |||
| assert(txn_db); | |||
| txn_db_impl_ = | |||
| static_cast_with_check<TransactionDBImpl, TransactionDB>(txn_db); | |||
| static_cast_with_check<PessimisticTxnDB, TransactionDB>(txn_db); | |||
There was a problem hiding this comment.
I guess multiple of places have PessimisticTxnDB instead of PessimisticTransactionDB
I think we will hit a compilation error
|
@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. |
1 similar comment
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This opens space for the new implementations of TransactionDBImpl such as WritePreparedTxnDBImpl that has a different policy of how to write to DB.