-
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
sst_dump recompress show #blocks compressed and not compressed #4835
Conversation
For: facebook#1474 Helps show when the 12.5% threshold for GoodCompressionRatio (originally from ldb) is hit.
For: facebook#1474 Helps show when the 12.5% threshold for GoodCompressionRatio (originally from ldb) is hit.
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.
It's great! Only one comment.
tools/sst_dump_tool.cc
Outdated
@@ -158,7 +158,8 @@ Status SstFileDumper::DumpTable(const std::string& out_filename) { | |||
} | |||
|
|||
uint64_t SstFileDumper::CalculateCompressedTableSize( | |||
const TableBuilderOptions& tb_options, size_t block_size) { | |||
const TableBuilderOptions& tb_options, size_t block_size, | |||
uint64_t& num_data_blocks) { |
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.
We follow Google C++ Style and it bans using reference as output argument: https://google.github.io/styleguide/cppguide.html#Reference_Arguments
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, I changed it to a pointer.
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 @thatsafunnyname for the contribution. I've left some comments.
@@ -188,6 +189,7 @@ uint64_t SstFileDumper::CalculateCompressedTableSize( | |||
exit(1); | |||
} | |||
uint64_t size = table_builder->FileSize(); | |||
*num_data_blocks = table_builder->GetTableProperties().num_data_blocks; |
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.
Maybe add assert(num_data_blocks != nullptr)
to make static checker happy?
const uint64_t not_compressed_blocks = | ||
opts.statistics->getAndResetTickerCount(NUMBER_BLOCK_NOT_COMPRESSED); | ||
if( (compressed_blocks + not_compressed_blocks) > num_data_blocks ){ | ||
num_data_blocks = compressed_blocks + not_compressed_blocks; |
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.
Could you add a comment on when (compressed_blocks + not_compressed_blocks) > num_data_blocks
is true? Thanks
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.
I added a comment, it is when the option enable_index_compression is true.
I restarted the failing travis-lite build. This should be fixed, hopefully. |
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 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@thatsafunnyname Some errors when building it:
I guess master has changed, so this needs to be accessed through a setter. If you fix it, I can land it. |
@thatsafunnyname has updated the pull request. Re-import the pull request |
Closing in favour of #5791 which has been updated against master and is using a setter to set the stats level. |
Closes #1474
Helps show when the 12.5% threshold for GoodCompressionRatio (originally from ldb) is hit.
Example output: