Fix blob db compression bug #2447
Conversation
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
utilities/blob_db/blob_db_impl.cc
Outdated
| std::string compression_output; | ||
| if (impl_->bdb_options_.compression != kNoCompression) { | ||
| if (impl_->bdb_options_.compression == kNoCompression) { |
There was a problem hiding this comment.
In lines 924 and 1058, wondering if you can do something like Slice value = GetFooBarValue(value_unc, compression, ...) and move the common code into that function?
There was a problem hiding this comment.
great suggestion!
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -1042,6 +1036,18 @@ Status BlobDBImpl::PutWithTTL(const WriteOptions& options, | |||
| : -1); | |||
| } | |||
|
|
|||
| Slice BlobDBImpl::GetCompressedSlice(const Slice& raw, | |||
| std::string* compression_output) const { | |||
There was a problem hiding this comment.
I believe you just overlooked this issue while trying to make changes rapidly 😃
No need to pass in compression_output as you are returning it; just declare it locally in this function. Also lines 925 and 1061 can be removed.
There was a problem hiding this comment.
Good to go once this change is made.
There was a problem hiding this comment.
compression_output is needed. It backs the return Slice, and have to be destroy after we finish using the value. I need to fix compile error though..
There was a problem hiding this comment.
ah, didn't realize that the storage is external to Slice. Good point.
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
yiwu-arbug
force-pushed
the
blob_compression
branch
from
6389e97
to
aa9728a
Compare
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: `CompressBlock()` will return the uncompressed slice (i.e. `Slice(value_unc)`) if compression ratio is not good enough. This is undesired. We need to always assign the compressed slice to `value`. Closes #2447 Differential Revision: D5244682 Pulled By: yiwu-arbug fbshipit-source-id: 6989dd8852c9622822ba9acec9beea02007dff09
Summary: `CompressBlock()` will return the uncompressed slice (i.e. `Slice(value_unc)`) if compression ratio is not good enough. This is undesired. We need to always assign the compressed slice to `value`. Closes #2447 Differential Revision: D5244682 Pulled By: yiwu-arbug fbshipit-source-id: 6989dd8852c9622822ba9acec9beea02007dff09
Summary:
CompressBlock()will return the uncompressed slice (i.e.Slice(value_unc)) if compression ratio is not good enough. This is undesired. We need to always assign the compressed slice tovalue.Test Plan:
Test with
BlobDBTest::Compressiontest which can be found in #2446