-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support parallel read and write/delete to same key in NonBatchedOpsStressTest #11058
Conversation
f703d6a
to
ed8c324
Compare
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
ed8c324
to
21ab545
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
21ab545
to
9a53dd5
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
9a53dd5
to
06e5d7e
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. There are a few ways to improve readability but overall it's already pretty nice to read - thanks for the great PR!
Thanks for the review - will get back to this next week |
06e5d7e
to
4a7ae09
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
4a7ae09
to
3d57beb
Compare
d5ab031
to
7841783
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
7841783
to
8ed8109
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
8ed8109
to
756af42
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
756af42
to
5440fca
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the updates
@hx235 has updated the pull request. You must reimport the pull request before landing. |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: **Context/Summary** After #11058, we no longer lock the key range to iterate in TestIterateAgainstExpected, except for working with timestamp feature. Pull Request resolved: #11695 Test Plan: no code change Reviewed By: ajkr Differential Revision: D48276668 Pulled By: hx235 fbshipit-source-id: dc92a3708b2281dc737c0877fb755548bf03a9fc
Summary: **Context/Summary** After facebook/rocksdb#11058, we no longer lock the key range to iterate in TestIterateAgainstExpected, except for working with timestamp feature. Pull Request resolved: facebook/rocksdb#11695 Test Plan: no code change Reviewed By: ajkr Differential Revision: D48276668 Pulled By: hx235 fbshipit-source-id: dc92a3708b2281dc737c0877fb755548bf03a9fc
Summary: This is most likely copypasta from `TestGet` from before facebook#11058 . There is no need to lock the mutex for the key for reads; in fact, doing so is detrimental to test coverage since it locks out concurrent writers. Differential Revision: D57915207
…ity (facebook#12709) Summary: This is most likely copypasta from `TestGet` from before facebook#11058 . There is no need to lock the mutex for the key for reads; in fact, doing so is detrimental to test coverage since it locks out concurrent writers. Differential Revision: D57915207
…ity (#12709) Summary: Pull Request resolved: #12709 This is most likely copypasta from `TestGet` from before #11058 . There is no need to lock the mutex for the key for reads; in fact, doing so is detrimental to test coverage since it locks out concurrent writers. Reviewed By: jowlyzhang Differential Revision: D57915207 fbshipit-source-id: eb0dbf6b84e5408b87d96dd47597511996e206a7
Summary: **Context/Summary** After facebook/rocksdb#11058, we no longer lock the key range to iterate in TestIterateAgainstExpected, except for working with timestamp feature. Pull Request resolved: facebook/rocksdb#11695 Test Plan: no code change Reviewed By: ajkr Differential Revision: D48276668 Pulled By: hx235 fbshipit-source-id: dc92a3708b2281dc737c0877fb755548bf03a9fc
Context:
Current
NonBatchedOpsStressTest
does not allow multi-thread read (i.e, Get, Iterator) and write (i.e, Put, Merge) or delete to the same key. Every read or write/delete operation will acquire lock (GetLocksForKeyRange
) on the target key to gain exclusive access to it. This does not align with RocksDB's nature of allowing multi-thread read and write/delete to the same key, that is concurrent threads can issue read/write/delete to RocksDB without external locking. Therefore this is a gap in our testing coverage.To close the gap, biggest challenge remains in verifying db value against expected state in presence of parallel read and write/delete. The challenge is due to read/write/delete to the db and read/write to expected state is not within one atomic operation. Therefore we may not know the exact expected state of a certain db read, as by the time we read the expected state for that db read, another write to expected state for another db write to the same key might have changed the expected state.
Summary:
Credited to @ajkr's idea, we now solve this challenge by breaking the 32-bits expected value of a key into different parts that can be read and write to in parallel.
Basically we divide the 32-bits expected value into
value_base
(corresponding to the previous whole 32 bits but now with some shrinking in the value base range we allow),pending_write
(i.e, whether there is an ongoing concurrent write),del_counter
(i.e, number of times a value has been deleted, analogous to value_base for write),pending_delete
(similar to pending_write) anddeleted
(i.e whether a key is deleted).Also, we need to use incremental
value_base
instead of random value base as before because we want to control the range of value base a correct db read result can possibly be in presence of parallel read and write. In that way, we can verify the correctness of the read against expected state more easily. This is at the cost of reducing the randomness of the value generated in NonBatchedOpsStressTest we are willing to accept.(For detailed algorithm of how to use these parts to infer expected state of a key, see the PR)
Misc: hide value_base detail from callers of ExpectedState by abstracting related logics into ExpectedValue class
Test: