-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Deep-copy Options in restored db for stress test to avoid race with SetOptions() #12015
Conversation
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Looking at this |
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.
Otherwise, LGTM, thanks!
Status s; | ||
ConfigOptions config_opts; | ||
|
||
std::string db_options_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.
Outside the context of this PR, these steps appear to be doing nothing useful. There should be a clear comment on why it's there.
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.
Fixed
f63f5b7
to
25760c4
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. |
Context
DB open will persist the
Options
in memory to options file and verify the file right after the write. The verification is done by comparing the options from parsing the written options file against theOptions
object in memory. Upon inconsistency, corruption such as https://github.com/facebook/rocksdb/blob/main/options/options_parser.cc#L725 will be returned.This verification assumes the
Options
object in memory is not changed from before the write till the verification. This assumption can break during opening the restored db in stress test.This line makes it shares some pointer options (e.g,
std::shared_ptr<const FilterPolicy> filter_policy
) with other threads (e.g, SetOptions()) in db stress.And since #11838, filter_policy's field
bloom_before_level
has now been mutable by SetOptions(). Therefore we started to see stress test failure like below:Summary
This PR uses "deep copy" of the
options_
by CreateXXXFromString() to avoid sharing pointer options.Test plan
Run the below db stress command that failed before this PR and pass after