-
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
Expand Statistics support in the C API #11263
Conversation
waiting_skeleton.jpg |
db/c_test.c
Outdated
CheckNoError(err); | ||
|
||
rocksdb_options_enable_statistics(options); | ||
rocksdb_options_set_statistics_level(options, 5); |
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.
Similar suggestion here: use static_cast<uint8_t>(StatsLevel::kAll), assuming by "5", you want to mean StatsLevel::kAll
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.
Please see my #11263 (comment)
CheckCondition(0 != rocksdb_options_statistics_get_ticker_count( | ||
options, BYTES_WRITTEN_TICKER)); | ||
rocksdb_options_statistics_get_histogram_data(options, DB_WRITE_HIST, hist); | ||
CheckCondition(0 != rocksdb_statistics_histogram_data_get_sum(hist)); |
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.
Can we also check for other fields rocksdb_statistics_histogram_data_get_*(hist)
to be non-zero?
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.
Thanks for working on this! I left a few comments
@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.
Thanks for addressing the previous comments - most of them are resolved. The rest of them, including the follow-up and the open ones, should be easy to address I believe. Thanks!
Added functions for changing statistics level and collecting tickers and histograms programmatically via the C API
8549da0
to
d68bc6f
Compare
@amatveev-cf has updated the pull request. You must reimport the pull request before landing. |
@amatveev-cf 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. |
@amatveev-cf has updated the pull request. You must reimport the pull request before landing. |
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! Thank you for addressing the comments!!!
@amatveev-cf 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. |
Adds a few missing features to the C API:
Test Plan: unit tests