Skip to content

Refactor and clean up the code that reads a blob from a file - #6093

Closed
ltamasi wants to merge 2 commits into
meta:masterfrom
ltamasi:blob_db_get_raw_blob
Closed

ltamasi wants to merge 2 commits into
meta:masterfrom
ltamasi:blob_db_get_raw_blob

Conversation

@ltamasi

@ltamasi ltamasi commented Nov 26, 2019 •

Copy link
Copy Markdown
Contributor

Summary:
This patch factors out the logic that reads a (potentially compressed) blob
from a file into a separate helper method GetRawBlobFromFile, and cleans
up the code a bit. Also, errors during decompression are now logged/propagated
to the user by returning a Status code of Corruption.

Test Plan:
make check

@facebook-github-bot facebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@facebook-github-bot facebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@ltamasi ltamasi changed the title [WIP] Refactor and clean up the code that reads blobs from files a bit, handle blob uncompression errors Refactor and clean up the code that reads a blob from files a bit, handle blob uncompression errors Nov 26, 2019
@ltamasi ltamasi changed the title Refactor and clean up the code that reads a blob from files a bit, handle blob uncompression errors Refactor and clean up the code that reads a blob from a file Nov 26, 2019
@ltamasi
ltamasi requested a review from siying November 26, 2019 23:51
compression_type);
s = UncompressBlockContentsForCompressionType(
info, value->data(), value->size(), &contents,
kBlockBasedTableVersionFormat, *(cfh->cfd()->ioptions()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's outside the scope to this PR but UncompressBlockContentsForCompressionType() seems to only need ImmutableCFOptions for Env and Statistics, which should be available here locally too. The complicated casting for cfh should be able to be avoided.

}

if (!s.ok()) {
if (debug_level_ >= 2) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is also outside the scope of the PR but I don't understand why blobDB doesn't rely on the standard RocksDB log level system, but use another debug level. This is unnecessarily complicating things to me.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@ltamasi merged this pull request in d9314a9.

dvdplm added a commit to dvdplm/rocksdb that referenced this pull request Nov 27, 2019
* upstream/master: (572 commits)
  Work around weird unused errors with Mingw (meta#6075)
  Support options.max_open_files = -1 with periodic_compaction_seconds (meta#6090)
  Fix HISTORY.md for 6.6.0 (meta#6096)
  Expose and elaborate FilterBuildingContext (meta#6088)
  Fix compilation under MSVC VS2015 (meta#6081)
  Add shared library for musl-libc (meta#3143)
  Refactor and clean up the code that reads a blob from a file (meta#6093)
  Allow fractional bits/key in BloomFilterPolicy (meta#6092)
  Refactor blob file creation logic (meta#6066)
  Use lowercase for shlwapi.lib rpcrt4.lib (meta#6076)
  Fix naming of library on PPC64LE (meta#6080)
  Small improvements to Docker build for RocksJava (meta#6079)
  Remove unused/undefined ImmutableCFOptions() (meta#6086)
  Update 3rd-party libraries used by RocksJava (meta#6084)
  Make default value of options.ttl to be 30 days when it is supported. (meta#6073)
  Ignore value of BackupableDBOptions::max_valid_backups_to_open when B… (meta#6072)
  Update HISTORY.md for forward compatibility (meta#6085)
  Support ttl in Universal Compaction (meta#6071)
  Fix the constness issues around autovector::iterator_impl's dereference operators (meta#6057)
  Support options.ttl with options.max_open_files = -1 (meta#6060)
  ...
wolfkdy pushed a commit to wolfkdy/rocksdb that referenced this pull request Dec 22, 2019
Summary:
This patch factors out the logic that reads a (potentially compressed) blob
from a file into a separate helper method `GetRawBlobFromFile`, and cleans
up the code a bit. Also, errors during decompression are now logged/propagated
to the user by returning a `Status` code of `Corruption`.

Pull Request resolved: meta#6093

Test Plan: `make check`

Differential Revision: D18716673

Pulled By: ltamasi

fbshipit-source-id: 44144bc064cab616862d5643f34384f2bae6eb78
Tim-Reed pushed a commit to Tim-Reed/rocksdb that referenced this pull request Oct 1, 2026
Summary:
This patch factors out the logic that reads a (potentially compressed) blob
from a file into a separate helper method `GetRawBlobFromFile`, and cleans
up the code a bit. Also, errors during decompression are now logged/propagated
to the user by returning a `Status` code of `Corruption`.

Pull Request resolved: meta#6093

Test Plan: `make check`

Differential Revision: D18716673

Pulled By: ltamasi

fbshipit-source-id: 44144bc064cab616862d5643f34384f2bae6eb78
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants