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.

Sign up for GitHub

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

Check a status in InstallOutputBlobFiles and Rewrite BatchWriteNewIndices #243

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Check a status in InstallOutputBlobFiles and Rewrite BatchWriteNewIndices #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d0b42f
Add checking a status in BlobGCJob::InstallOutputBlobFiles
apple-ouyang May 4, 2022
187a55b
Rewritte the BatchWriteNewIndices
apple-ouyang May 4, 2022
8740cfa
Avoid unnecessary MergeBlobIndex when gc_merge_rewrite = false
apple-ouyang May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 22 additions & 19 deletions src/blob_gc_job.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ Status BlobGCJob::DoRunGC() {
BlobFileBuilder::OutContexts contexts;
blob_file_builder->Add(blob_record, std::move(ctx), &contexts);

BatchWriteNewIndices(contexts, &s);
s = BatchWriteNewIndices(contexts);

if (!s.ok()) {
break;
Expand All @@ -269,46 +269,46 @@ Status BlobGCJob::DoRunGC() {
return s;
}

void BlobGCJob::BatchWriteNewIndices(BlobFileBuilder::OutContexts& contexts,
Status* s) {
Status BlobGCJob::BatchWriteNewIndices(const BlobFileBuilder::OutContexts& contexts) {
auto* cfh = blob_gc_->column_family_handle();
for (const std::unique_ptr<BlobFileBuilder::BlobRecordContext>& ctx :
contexts) {
MergeBlobIndex merge_blob_index;
merge_blob_index.file_number = ctx->new_blob_index.file_number;
merge_blob_index.source_file_number = ctx->original_blob_index.file_number;
merge_blob_index.source_file_offset =
ctx->original_blob_index.blob_handle.offset;
merge_blob_index.blob_handle = ctx->new_blob_index.blob_handle;

Status s;
for (const auto& ctx : contexts) {
std::string index_entry;
BlobIndex original_index = ctx->original_blob_index;
ParsedInternalKey ikey;
if (!ParseInternalKey(ctx->key, &ikey)) {
*s = Status::Corruption(Slice());
return;
return Status::Corruption(Slice());;
Copy link
Member

Choose a reason for hiding this comment

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

add a meaning error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll add one

}
if (!gc_merge_rewrite_) {
merge_blob_index.EncodeToBase(&index_entry);
const BlobIndex &index = ctx->new_blob_index;
Copy link
Member

Choose a reason for hiding this comment

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

why is it different from before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the merge blob index is only used in the second if branch. I just think we should only declare the merge blob index when we truly use it. This will help others understand the code easily.

index.EncodeTo(&index_entry);
// Store WriteBatch for rewriting new Key-Index pairs to LSM
GarbageCollectionWriteCallback callback(cfh, ikey.user_key.ToString(),
std::move(original_index));
callback.value = index_entry;
rewrite_batches_.emplace_back(
std::make_pair(WriteBatch(), std::move(callback)));
auto& wb = rewrite_batches_.back().first;
*s = WriteBatchInternal::PutBlobIndex(&wb, cfh->GetID(), ikey.user_key,
s = WriteBatchInternal::PutBlobIndex(&wb, cfh->GetID(), ikey.user_key,
index_entry);
} else {
MergeBlobIndex merge_blob_index;
merge_blob_index.file_number = ctx->new_blob_index.file_number;
merge_blob_index.source_file_number =
ctx->original_blob_index.file_number;
merge_blob_index.source_file_offset =
ctx->original_blob_index.blob_handle.offset;
merge_blob_index.blob_handle = ctx->new_blob_index.blob_handle;
merge_blob_index.EncodeTo(&index_entry);
rewrite_batches_without_callback_.emplace_back(
std::make_pair(WriteBatch(), original_index.blob_handle.size));
auto& wb = rewrite_batches_without_callback_.back().first;
*s = WriteBatchInternal::Merge(&wb, cfh->GetID(), ikey.user_key,
s = WriteBatchInternal::Merge(&wb, cfh->GetID(), ikey.user_key,
index_entry);
}
if (!s->ok()) break;
if (!s.ok()) break;
}
return s;
}

Status BlobGCJob::BuildIterator(
Expand Down Expand Up @@ -415,7 +415,10 @@ Status BlobGCJob::InstallOutputBlobFiles() {
for (auto& builder : blob_file_builders_) {
BlobFileBuilder::OutContexts contexts;
s = builder.second->Finish(&contexts);
BatchWriteNewIndices(contexts, &s);
if (!s.ok()) {
break;
}
s = BatchWriteNewIndices(contexts);
if (!s.ok()) {
break;
}
Expand Down
2 changes: 1 addition & 1 deletion src/blob_gc_job.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class BlobGCJob {
uint64_t io_bytes_written_ = 0;

Status DoRunGC();
void BatchWriteNewIndices(BlobFileBuilder::OutContexts &contexts, Status *s);
Status BatchWriteNewIndices(const BlobFileBuilder::OutContexts& contexts);
Status BuildIterator(std::unique_ptr<BlobFileMergeIterator> *result);
Status DiscardEntry(const Slice &key, const BlobIndex &blob_index,
bool *discardable);
Expand Down