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

Jump to bottom

Fix a bug in ReFitLevel() where FileMetaData::being_compacted is not cleared #13009

Closed
wants to merge 3 commits into from
Closed

Fix a bug in ReFitLevel() where FileMetaData::being_compacted is not cleared #13009

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
6718946
fix
cbi42 Sep 12, 2024
fe58a32
add change log
cbi42 Sep 12, 2024
c612386
small refactor
cbi42 Sep 12, 2024
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
9 changes: 4 additions & 5 deletions db/compaction/compaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,12 +686,11 @@ bool Compaction::KeyRangeNotExistsBeyondOutputLevel(
};

// Mark (or clear) each file that is being compacted
void Compaction::MarkFilesBeingCompacted(bool mark_as_compacted) {
void Compaction::MarkFilesBeingCompacted(bool being_compacted) const {
for (size_t i = 0; i < num_input_levels(); i++) {
for (size_t j = 0; j < inputs_[i].size(); j++) {
assert(mark_as_compacted ? !inputs_[i][j]->being_compacted
: inputs_[i][j]->being_compacted);
inputs_[i][j]->being_compacted = mark_as_compacted;
assert(being_compacted != inputs_[i][j]->being_compacted);
inputs_[i][j]->being_compacted = being_compacted;
}
}
}
Expand Down Expand Up @@ -735,7 +734,7 @@ uint64_t Compaction::CalculateTotalInputSize() const {
return size;
}

void Compaction::ReleaseCompactionFiles(Status status) {
void Compaction::ReleaseCompactionFiles(const Status& status) {
MarkFilesBeingCompacted(false);
cfd_->compaction_picker()->ReleaseCompactionFiles(this, status);
}
Expand Down
8 changes: 4 additions & 4 deletions db/compaction/compaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ class Compaction {
// Delete this compaction from the list of running compactions.
//
// Requirement: DB mutex held
void ReleaseCompactionFiles(Status status);
void ReleaseCompactionFiles(const Status& status);

// Returns the summary of the compaction in "output" with maximum "len"
// in bytes. The caller is responsible for the memory management of
Expand Down Expand Up @@ -435,13 +435,13 @@ class Compaction {
const int start_level,
const int output_level);

// mark (or clear) all files that are being compacted
void MarkFilesBeingCompacted(bool being_compacted) const;

private:

Status InitInputTableProperties();

// mark (or clear) all files that are being compacted
void MarkFilesBeingCompacted(bool mark_as_compacted);

// get the smallest and largest key present in files to be compacted
static void GetBoundaryKeys(VersionStorageInfo* vstorage,
const std::vector<CompactionInputFiles>& inputs,
Expand Down
3 changes: 2 additions & 1 deletion db/compaction/compaction_picker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ CompactionPicker::CompactionPicker(const ImmutableOptions& ioptions,
CompactionPicker::~CompactionPicker() = default;

// Delete this compaction from the list of running compactions.
void CompactionPicker::ReleaseCompactionFiles(Compaction* c, Status status) {
void CompactionPicker::ReleaseCompactionFiles(Compaction* c,
const Status& status) {
UnregisterCompaction(c);
if (!status.ok()) {
c->ResetNextCompactionIndex();
Expand Down
2 changes: 1 addition & 1 deletion db/compaction/compaction_picker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class CompactionPicker {
// Free up the files that participated in a compaction
//
// Requirement: DB mutex held
void ReleaseCompactionFiles(Compaction* c, Status status);
void ReleaseCompactionFiles(Compaction* c, const Status& status);

// Returns true if any one of the specified files are being compacted
bool AreFilesInCompaction(const std::vector<FileMetaData*>& files);
Expand Down
2 changes: 1 addition & 1 deletion db/db_impl/db_impl_compaction_flush.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1880,7 +1880,7 @@ Status DBImpl::ReFitLevel(ColumnFamilyData* cfd, int level, int target_level) {
Status status = versions_->LogAndApply(cfd, mutable_cf_options,
read_options, write_options, &edit,
&mutex_, directories_.GetDbDir());

c->MarkFilesBeingCompacted(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seem like an opportunity for future tech debt to bundle related book-keeping together (e.g, release,unregister,MarkFilesBeingCompacted(false), destructor stuff) ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to use Compaction::ReleaseCompactionFiles() here which is more consistent with other places. It failed since in ReFitLevel() we don't call Compaction::FinalizeInputInfo() which sets cfd_ in the compaction, while ReleaseCompactionFiles uses cfd_.

cfd->compaction_picker()->UnregisterCompaction(c.get());
c.reset();

Expand Down
1 change: 1 addition & 0 deletions unreleased_history/bug_fixes/bug-refit-level.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Fix a bug in CompactRange() where result files may not be compacted in any future compaction. This can only happen when users configure CompactRangeOptions::change_level to true and the change level step of manual compaction fails (#13009).
Loading