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

Return Aborted when no valid input files are found for a CompactFile call #12633

Closed
wants to merge 2 commits into from
Closed

Return Aborted when no valid input files are found for a CompactFile call #12633

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac29015
Add check and assertion for CompactFiles
jowlyzhang May 9, 2024
c30c418
Address review comments
jowlyzhang May 9, 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
15 changes: 12 additions & 3 deletions db/compaction/compaction_picker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,8 +920,9 @@ Status CompactionPicker::SanitizeCompactionInputFilesForAllLevels(
// the smallest and largest key of the current compaction input
std::string smallestkey;
std::string largestkey;
bool valid_input_files_found = false;
// a flag for initializing smallest and largest key
bool is_first = false;
bool is_first = true;
const int kNotFound = -1;

// For each level, it does the following things:
Expand All @@ -946,10 +947,11 @@ Status CompactionPicker::SanitizeCompactionInputFilesForAllLevels(
}
first_included = std::min(first_included, static_cast<int>(f));
last_included = std::max(last_included, static_cast<int>(f));
if (is_first == false) {
if (is_first) {
smallestkey = current_files[f].smallestkey;
largestkey = current_files[f].largestkey;
is_first = true;
valid_input_files_found = true;
is_first = false;
}
}
if (last_included == kNotFound) {
Expand Down Expand Up @@ -1040,6 +1042,13 @@ Status CompactionPicker::SanitizeCompactionInputFilesForAllLevels(
}
}
}

if (!valid_input_files_found) {
Copy link
Member

@cbi42 cbi42 May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Does checking is_first works here? Or rename is_first to valid_input_files_found for better naming.

return Status::Aborted(
"There are no valid input files found for the specified files.");
}
assert(!smallestkey.empty());
Copy link
Member

Choose a reason for hiding this comment

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

I think empty user key is allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't quite get this, in the RangeOverlapWithCompaction function, empty user key can cause assertions in this logic:

rocksdb/db/compaction/compaction_picker.cc

Line 266 in 9bddac0

ucmp->CompareWithoutTimestamp(smallest_user_key,

So I think it's better to not pass empty boundaries to that function yet. These user key boundaries come from the existing files' boundaries. At this point, they should either represent some file's actual range, or no valid files found and they are empty. So here we want to not check further if range overlap when it's empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I didn't get that you meant Put("","") is allowed. Thanks for catching this!

assert(!largestkey.empty());
if (RangeOverlapWithCompaction(smallestkey, largestkey, output_level)) {
return Status::Aborted(
"A running compaction is writing to the same output level in an "
Expand Down
16 changes: 11 additions & 5 deletions db/db_with_timestamp_basic_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,17 @@ TEST_F(DBBasicTestWithTimestamp, CompactRangeWithSpecifiedRange) {
ASSERT_OK(db_->Put(write_opts, "foo2", ts, "bar"));
ASSERT_OK(Flush());

std::string start_str = "foo";
std::string end_str = "foo2";
Slice start(start_str), end(end_str);
ASSERT_OK(db_->CompactRange(CompactRangeOptions(), &start, &end));

port::Thread compact_range([&]() {
std::string start_str = "foo";
std::string end_str = "foo2";
Slice start(start_str), end(end_str);
ASSERT_OK(db_->CompactRange(CompactRangeOptions(), &start, &end));
});
// Compact random(non-existing) file receive a proper status.
ASSERT_TRUE(db_->CompactFiles(CompactionOptions(), db_->DefaultColumnFamily(),
{"002349.sst"}, 1)
.IsAborted());
compact_range.join();
Close();
}

Expand Down
Loading