-
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
Improve stress test for transactions #9568
Conversation
f28cd93
to
333614c
Compare
936f45c
to
4e73cb8
Compare
c3d67cf
to
a60a865
Compare
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
cc @lth |
a60a865
to
e19c729
Compare
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
aca55bf
to
9fe0efd
Compare
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
9fe0efd
to
3c38047
Compare
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
@riversand963 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
// location, and this file will be used by future runs until a new db is | ||
// created. | ||
// | ||
// Create a fresh new dataabse (-destroy_db_initially=1 or there is no database |
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.
typo database
DEFINE_int32(lb_a, 0, "(Inclusive) lower bound of A"); | ||
DEFINE_int32(ub_a, 1000, "(Exclusive) upper bound of A"); | ||
DEFINE_int32(lb_c, 0, "(Inclusive) lower bound of C"); | ||
DEFINE_int32(ub_c, 1000, "(Exclusive) upper bound of C"); | ||
|
||
DEFINE_string(key_spaces_path, "", | ||
"Path to file describing the lower and upper bounds of A and C"); |
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.
What are A
key and C
key? are they myrocks keys from https://github.com/facebook/mysql-5.6/wiki/MyRocks-record-format
I think we should explain the key format here and how to choose it, why the key range is int
.
And we might want to name the stress test blackbox_crash_test_myrocks_multiops_wc
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.
The key format is described in multi_ops_txns_stress.h. I will put a comment here pointing to the header.
The stress test is not only for write-committed (wc)
std::ostringstream oss; | ||
oss << "pk should exist: " << Slice(pk).ToString(true); | ||
fprintf(stderr, "%s\n", oss.str().c_str()); |
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.
I'm wondering if we should use std::cerr
instead (maybe a future refactor PR)
// Both [lb_a, ub_a) and [lb_c, ub_c) are partitioned. Each thread operates on | ||
// one sub range, using KeyGenerators to generate keys. |
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.
If each thread is only operating on its own partition, does that mean we're not testing concurrent operations on the same key? Is it to mimic myrocks operation which only has single thread for a key-range?
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.
Even if multiple threads are operating on overlapping key ranges, at the db layer, there will never be concurrent accesses to the same key due to locking at Transaction layer. Having each thread operate on disjoint key ranges makes it easier for us to manage the existing and non-existing keys across different db restarts. This is necessary if we want the majority of read to actually find the keys instead of return not-found, in which case, we are not stressing the database as much.
// data specified via `-lb_a`, `-ub_a`, `-lb_c`, `-ub_c`, etc. Among these, we | ||
// define the test key spaces as two key ranges: [lb_a, ub_a) and [lb_c, ub_c). | ||
// The key spaces specification is persisted in a file whose absolute path can | ||
// be specified via `-key_spaces_path`. |
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.
Key spaces specification are just 4 integers, right? Why do we want to persist it in a file instead of providing it from cmd?
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.
Right.
I feel that the key spaces should be persisted across database restarts if destroy_db_initially
is false. This is different from the ranges of keys that actually exist in the database, thus we cannot rely on checking the db for this info.
3c38047
to
26f14e3
Compare
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thanks @jay-zhuang for the review! |
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This reverts commit f28cd93d680466d4f4190a0d2c19d7c10548bca3.
Summary: Add some extra assertions assuming key spaces and test thread count remain the same across different runs. Will need to relax this in the future.
Summary: Will re-enable other options later
26f14e3
to
5597354
Compare
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Test only, no change to functionality.
Extremely low risk of library regression.
Update test key generation by maintaining existing and non-existing keys.
Update db_crashtest.py to drive multiops_txn stress test for both write-committed and write-prepared.
Add a make target 'blackbox_crash_test_with_multiops_txn'.
Running the following commands caught the bug exposed in #9571.
Running the following command caught a bug which will be fixed in #9648 .