parallel occ #6240
parallel occ #6240
Conversation
2) modify unittests to test both modes 3) make format 4) refine hash functor
|
continuous-integration/appveyor/pr says it runs for more than 90 mins and exceeded the quota. |
| // write-group). | ||
| // May suffer from high mutex contention, as per this link: | ||
| // https://github.com/facebook/rocksdb/issues/4402 | ||
| VALIDATE_SERIAL = 0, |
There was a problem hiding this comment.
Our naming convenstion is kValidateSerial.
There was a problem hiding this comment.
Thank you for your contribution! I have some final comments. Most of them are about code styles. We are almost ready to go. Please mention the improvement in HISTORY.md.
thanks for your patience. I've resolved this. you can find it in HISTORY.md in the diff.
There was a problem hiding this comment.
Our naming convenstion is
kValidateSerial.
resolved
| // reduce mutex contention. | ||
| // Each txn acquires locks for its write-set records in some well-defined | ||
| // order. | ||
| VALIDATE_PARALLEL |
There was a problem hiding this comment.
Since we have number assigned in the previous value. It feels natural to assign both.
| OccValidationPolicy validate_policy = OccValidationPolicy::VALIDATE_PARALLEL; | ||
|
|
||
| // NOTE(wolfkdy): works only if validate_policy == | ||
| // OccValidationPolicy::VALIDATE_PARALLEL |
There was a problem hiding this comment.
What does NOTE(wolfkdy) mean?
.h files under include/rocksdb is visible not only to RocksDB developers, but users too. So we have higher standard to comments here. Please improve it a little bit.
There was a problem hiding this comment.
ummm, wolfkdy is my github's id. I used to add name after NOTE tag for potential conversations.
I've removed this to match rocksdb's standard. You can find it in the latest commit.
resolved.
| }; | ||
|
|
||
| struct OptimisticTransactionDBOptions { | ||
| OccValidationPolicy validate_policy = OccValidationPolicy::VALIDATE_PARALLEL; |
There was a problem hiding this comment.
Default behavior change needs to be explained in HISTORY.md.
| } | ||
| for (auto v : lk_idxes) { | ||
| lks.emplace_back(txn_db_impl->LockBucket(v)); | ||
| } |
There was a problem hiding this comment.
I remember there used to be some comments explaining locking order but I can't find it anymore. Please add back the comment about how deadlock is prevented here.
There was a problem hiding this comment.
some explanations have been added back for this in the latest commit.
| private: | ||
| // NOTE(wolfkdy): used in validation phase. Each key is hashed into some |
There was a problem hiding this comment.
Do we need NOTE(wolfkdy) here?
There was a problem hiding this comment.
wolfkdy has been removed to match rocksdb's standard.
|
@siying Thanks for your patience. I've resolved all the above issues. Would you plz take a look? |
| auto txn_db_impl = static_cast_with_check<OptimisticTransactionDBImpl, | ||
| OptimisticTransactionDB>(txn_db_); | ||
| assert(txn_db_impl); | ||
| OccValidationPolicy policy = txn_db_impl->GetValidatePolicy(); |
There was a problem hiding this comment.
Not big deal, but I feel it slightly more readable by dong a switch(txn_db_impl->GetValidatePolicy()) { case....
Considering the time zone difference, I'm not going to hold the PR because of it. But if you are interested, I encourage you to send a follow up PR for it.
There was a problem hiding this comment.
ummm, switch/case here seems more elegant.
I will make another pr in this week.
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request has been merged in 1ab1231. |
This is a continuation of https://github.com/facebook/rocksdb/pull/5320/files
I open a new mr for these purposes, half a year has past since the old mr is posted so it's almost impossible to fulfill some points below on the old mr, especially 5)