File level histogram should be printed per CF, not per DB #2126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
ajkr
approved these changes
Apr 11, 2017
include/rocksdb/db.h
Outdated
| // db's lifetime ("Sum"), and aggregated over the interval since the | ||
| // last retrieval ("Int"). | ||
| // "rocksdb.cfstats" - Both of "rocksdb.cf-file-histogram" and | ||
| // "rocksdb.cf-file-histogram" together. See below for description |
There was a problem hiding this comment.
rocksdb.cfstats-no-file-histogram
| void InternalStats::DumpCFFileHistogram(std::string* value) { | ||
| char buf[2000]; | ||
| snprintf(buf, sizeof(buf), | ||
| "\n** File Read Latency Histogram By Level [%s] **\n", |
There was a problem hiding this comment.
thanks for adding column family name, I think currently it doesn't specify.
Summary: Currently level histogram is only printed out for DB stats and for default CF. This is confusing. Change to print for every CF instead. Test Plan:Run all existing tests. Add unit test.
|
@siying updated the pull request - view changes - changes since last import |
|
Address the comment. |
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
mikhail-antonov
pushed a commit
to mikhail-antonov/rocksdb
that referenced
this pull request
Apr 12, 2017
Summary: Currently level histogram is only printed out for DB stats and for default CF. This is confusing. Change to print for every CF instead. Closes facebook#2126 Differential Revision: D4865373 Pulled By: siying fbshipit-source-id: 1c853e0ac66e00120ee931cabc9daf69ccc2d577
|
Actually I think per DB histogram is useful, Is there a reason for not including both ? |
|
@IslamAbdelRahman IIRC, the histogram that was printed before in dbstats was (misleadingly) the default column family's histogram. I don't think we've implemented per-DB histogram yet. |
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jun 10, 2021
Summary: DBImpl::DumpStats is supposed to do this: Dump DB stats to LOG For each CF, dump CFStatsNoFileHistogram to LOG For each CF, dump CFFileHistogram to LOG Instead, due to a longstanding bug from 2017 (facebook#2126), it would dump CFStats, which includes both CFStatsNoFileHistogram and CFFileHistogram, in both loops, resulting in near-duplicate output. This fixes the bug. Test Plan: Manual inspection of LOG
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 12, 2021
Summary: DBImpl::DumpStats is supposed to do this: Dump DB stats to LOG For each CF, dump CFStatsNoFileHistogram to LOG For each CF, dump CFFileHistogram to LOG Instead, due to a longstanding bug from 2017 (#2126), it would dump CFStats, which includes both CFStatsNoFileHistogram and CFFileHistogram, in both loops, resulting in near-duplicate output. This fixes the bug. Pull Request resolved: #8380 Test Plan: Manual inspection of LOG after db_bench Reviewed By: jay-zhuang Differential Revision: D29017535 Pulled By: pdillinger fbshipit-source-id: 3010604c4a629a80347f129cd746ce9b0d0cbda6
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jul 19, 2021
Summary: DBImpl::DumpStats is supposed to do this: Dump DB stats to LOG For each CF, dump CFStatsNoFileHistogram to LOG For each CF, dump CFFileHistogram to LOG Instead, due to a longstanding bug from 2017 (facebook#2126), it would dump CFStats, which includes both CFStatsNoFileHistogram and CFFileHistogram, in both loops, resulting in near-duplicate output. This fixes the bug. Pull Request resolved: facebook#8380 Test Plan: Manual inspection of LOG after db_bench Reviewed By: jay-zhuang Differential Revision: D29017535 Pulled By: pdillinger fbshipit-source-id: 3010604c4a629a80347f129cd746ce9b0d0cbda6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Currently level histogram is only printed out for DB stats and for default CF. This is confusing. Change to print for every CF instead.
Test Plan:Run all existing tests. Add unit test.