-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
Make statistics's stats_level change thread-safe #5030
Conversation
Summary: Right now, users can change statistics.stats_level while DB is running, but TSAN may report data race. We make stats_level_ to be atomic, and access them using accessors. Test Plan: Run existing tests.
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 except a comment and test failures.
include/rocksdb/statistics.h
Outdated
@@ -514,8 +514,16 @@ class Statistics { | |||
virtual bool HistEnabledForType(uint32_t type) const { | |||
return type < HISTOGRAM_ENUM_MAX; | |||
} | |||
void set_stats_level(StatsLevel sl) { | |||
stats_level_.store(sl, std::memory_order_relaxed); | |||
; |
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.
Remove 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.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
The java failure shows up occasionally in master too: https://travis-ci.org/facebook/rocksdb/builds/500087774 https://travis-ci.org/facebook/rocksdb/jobs/500079011 https://travis-ci.org/facebook/rocksdb/jobs/500066964 so I assume it's unrelated. |
Summary: Right now, users can change statistics.stats_level while DB is running, but TSAN may report data race. We make stats_level_ to be atomic, and access them using accessors. Pull Request resolved: facebook#5030 Differential Revision: D14267519 Pulled By: siying fbshipit-source-id: 37d7ebeff7a43a406230143422a16af899163f73
Minor changes in statistics -> facebook/rocksdb#5030 Related BBVA#114
Minor changes in statistics -> facebook/rocksdb#5030 Remove some functiones in our extended bindings which are no more required by this new version of Rocksdb Related BBVA#114
Minor changes in statistics -> facebook/rocksdb#5030 Remove some functiones in our extended bindings which are no more required by this new version of Rocksdb Related BBVA#114
Minor changes in statistics -> facebook/rocksdb#5030 Remove some functiones in our extended bindings which are no more required by this new version of Rocksdb Related BBVA#114
Minor changes in statistics -> facebook/rocksdb#5030 Remove some functiones in our extended bindings which are no more required by this new version of Rocksdb Related #114 Former-commit-id: 71e024c
Minor changes in statistics -> facebook/rocksdb#5030 Remove some functiones in our extended bindings which are no more required by this new version of Rocksdb Related #114 Former-commit-id: bfe2771 [formerly 71e024c] Former-commit-id: 3248422
Summary:
Right now, users can change statistics.stats_level while DB is running, but TSAN may report
data race. We make stats_level_ to be atomic, and access them using accessors.
Test Plan: Run existing tests.