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.

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

Change the semantics of bytes_read in GetBlob/MultiGetBlob for consistency #10248

Closed
wants to merge 10 commits into from

Conversation

gangliao
Copy link
Contributor

@gangliao gangliao commented Jun 23, 2022

Summary:

The bytes_read returned by the current BlobSource interface is ambiguous. The uncompressed blob size is returned if the cache hits. The size of the blob read from disk, presumably the compressed version, is returned if the cache misses. Two differing semantics might cause ambiguity and consistency issues. For example, this inconsistency causes the assertion failure (T124246362 and its hot fix is #10249).

This goal is to require that the value of byte read always be an on-disk blob record size.

@gangliao gangliao requested review from ltamasi and ajkr June 23, 2022 23:30
@gangliao gangliao mentioned this pull request Jun 23, 2022
14 tasks
db/blob/blob_source.cc Outdated Show resolved Hide resolved
@gangliao gangliao changed the title BlobDB in crash test hitting assertion Change the semantics of bytes_read in GetBlob/MultiGetBlob for consistency Jun 24, 2022
Summary:

This task is to fix assertion failures during the crash test runs.

```bash
db_stress: internal_repo_rocksdb/repo/db/blob/blob_source.cc:101: rocksdb::Status rocksdb::BlobSource::GetBlob(const rocksdb::ReadOptions &, const rocksdb::Slice &, uint64_t, uint64_t, uint64_t, uint64_t, rocksdb::CompressionType, rocksdb::FilePrefetchBuffer *, rocksdb::PinnableSlice *, uint64_t *): Assertion `blob_entry.GetValue()->size() == value_size' failed.
```

Tasks: T124246362
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_file_reader.h Outdated Show resolved Hide resolved
db/blob/blob_file_reader.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
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 updates @gangliao ! LGTM 👍

db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated 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
@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.

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.

None yet

3 participants