Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add blob cache tickers, perf context statistics, and DB properties #10203

Closed
wants to merge 12 commits into from
Closed

Add blob cache tickers, perf context statistics, and DB properties #10203

wants to merge 12 commits into from

Conversation

gangliao
Copy link
Contributor

@gangliao gangliao commented Jun 18, 2022 •
edited
Loading

Summary:

In order to be able to monitor the performance of the new blob cache, we made the follow changes:

This PR is a part of #10156

@gangliao gangliao mentioned this pull request Jun 18, 2022
Closed
14 tasks
@riversand963
Copy link
Contributor

The following can be removed from PR description

Reviewers:

Subscribers:

Tasks:

This PR is a part of https://github.com/facebook/rocksdb/issues/10156

Tags:

riversand963
riversand963 reviewed Jun 22, 2022
View reviewed changes
HISTORY.md Outdated Show resolved Hide resolved
java/rocksjni/portal.h Outdated Show resolved Hide resolved
akankshamahajan15
akankshamahajan15 reviewed Jun 23, 2022
View reviewed changes
include/rocksdb/statistics.h Outdated Show resolved Hide resolved
include/rocksdb/c.h Outdated Show resolved Hide resolved
@gangliao gangliao force-pushed the blob_statistics branch 2 times, most recently from 281e595 to b22a845 Compare June 27, 2022 19:27
gangliao
gangliao commented Jun 27, 2022
View reviewed changes
db/blob/blob_source.cc Outdated Show resolved Hide resolved
@gangliao
Copy link
Contributor Author

  • Add tickers/db properties/perf statistics to RocksDB WIKI.

riversand963
riversand963 approved these changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gangliao. Mostly LGTM.

include/rocksdb/statistics.h Show resolved Hide resolved
include/rocksdb/statistics.h Show resolved Hide resolved
gangliao added 9 commits June 27, 2022 17:55
@gangliao
Add blob cache tickers, perf context statistics, and DB properties
489a622 
Summary:

In order to be able to monitor the performance of the new blob cache, we made the follow changes:
- Add blob cache hit/miss/insertion tickers (see https://github.com/facebook/rocksdb/wiki/Statistics)
- Extend the perf context similarly (see https://github.com/facebook/rocksdb/wiki/Perf-Context-and-IO-Stats-Context)
- Implement new DB properties (see e.g. https://github.com/facebook/rocksdb/blob/main/include/rocksdb/db.h#L1042-L1051) that expose the capacity and current usage of the blob cache.

Test Plan:

Extend and update the unit tests.

Reviewers:

Subscribers:

Tasks:

Tags:
@gangliao
Fix typo
2eb373c
@gangliao
Fix alt-namespace status check
39002be
@gangliao
Skip blob cache property check when it's null
aedc0b1
@gangliao
Adjust the new ticket's position
291def8
@gangliao
Follow the comments
eaaf792
@gangliao
Fix SanityTickers
3e5ba02
@gangliao
Add tickers and perf contexts for multigetblob
cbfdb3f
@gangliao
Update perf counters
b5aa127
@facebook-github-bot
Copy link
Contributor

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@gangliao has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@gangliao has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ltamasi
ltamasi reviewed Jun 28, 2022
View reviewed changes
Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch @gangliao ! LGTM with some minor comments

HISTORY.md Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source_test.cc Show resolved Hide resolved
db/blob/blob_source_test.cc Outdated Show resolved Hide resolved
db/blob/blob_source_test.cc Outdated Show resolved Hide resolved
db/db_properties_test.cc Outdated Show resolved Hide resolved
db/internal_stats.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@gangliao has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ltamasi
ltamasi approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for updates!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akankshamahajan15 akankshamahajan15 akankshamahajan15 left review comments

@ltamasi ltamasi ltamasi approved these changes

@riversand963 riversand963 riversand963 approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gangliao @riversand963 @facebook-github-bot @akankshamahajan15 @ltamasi