Skip to content

Commit

Permalink
Add some asserts in FilePickerMultiGet for debugging (#12241)
Browse files Browse the repository at this point in the history
Summary:
Add asserts to help debug a crash test failure. The test fails as wollows -
```rocksdb::FilePickerMultiGet::PrepareNextLevel(): Assertion `fp_ctx.search_right_bound == -1 || fp_ctx.search_right_bound == FileIndexer::kLevelMaxIndex' failed```

Also add a unit test to verify an edge case.

Pull Request resolved: #12241

Reviewed By: cbi42

Differential Revision: D52819029

Pulled By: anand1976

fbshipit-source-id: 33316985c8ace1aed9ecc2400da8b777aec488ff
  • Loading branch information
anand1976 authored and facebook-github-bot committed Jan 17, 2024
1 parent c4228ab commit 65e162b
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 0 deletions.
50 changes: 50 additions & 0 deletions db/db_basic_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1853,6 +1853,56 @@ TEST_P(DBMultiGetTestWithParam, MultiGetBatchedMultiLevel) {
}
}

TEST_P(DBMultiGetTestWithParam, MultiGetBatchedEmptyLevel) {
#ifndef USE_COROUTINES
if (std::get<1>(GetParam())) {
ROCKSDB_GTEST_BYPASS("This test requires coroutine support");
return;
}
#endif // USE_COROUTINES
// Skip for unbatched MultiGet
if (!std::get<0>(GetParam())) {
ROCKSDB_GTEST_BYPASS("This test is only for batched MultiGet");
return;
}
Options options = CurrentOptions();
options.disable_auto_compactions = true;
options.merge_operator = MergeOperators::CreateStringAppendOperator();
Reopen(options);
int key;

key = 9;
ASSERT_OK(Put("key_" + std::to_string(key), "val_l2_" + std::to_string(key)));
ASSERT_OK(Flush());
MoveFilesToLevel(4);

key = 5;
ASSERT_OK(Put("key_" + std::to_string(key), "val_l2_" + std::to_string(key)));
key = 9;
ASSERT_OK(
Merge("key_" + std::to_string(key), "val_l2_" + std::to_string(key)));
ASSERT_OK(Flush());
// Leave level 3 empty
MoveFilesToLevel(2);

key = 2;
ASSERT_OK(Put("key_" + std::to_string(key), "val_l2_" + std::to_string(key)));
key = 6;
ASSERT_OK(
Merge("key_" + std::to_string(key), "val_l2_" + std::to_string(key)));
ASSERT_OK(Flush());
MoveFilesToLevel(1);

std::vector<std::string> keys;
std::vector<std::string> values;

keys.push_back("key_" + std::to_string(9));
keys.push_back("key_" + std::to_string(9));

values = MultiGet(keys, nullptr, std::get<1>(GetParam()));
ASSERT_EQ(values.size(), 2);
}

TEST_P(DBMultiGetTestWithParam, MultiGetBatchedMultiLevelMerge) {
#ifndef USE_COROUTINES
if (std::get<1>(GetParam())) {
Expand Down
15 changes: 15 additions & 0 deletions db/version_set.cc
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,19 @@ class FilePickerMultiGet {
// was not found, we need to skip lookup for the remaining keys and
// reset the search bounds
if (batch_iter_ != current_level_range_.end()) {
#ifndef NDEBUG
if (curr_level_ < num_levels_ + 1) {
if ((*level_files_brief_)[curr_level_].num_files == 0) {
struct FilePickerContext& fp_ctx =
fp_ctx_array_[batch_iter_.index()];

assert(fp_ctx.search_left_bound == 0);
assert(fp_ctx.search_right_bound == -1 ||
fp_ctx.search_right_bound == FileIndexer::kLevelMaxIndex);
}
}
#endif // NDBEUG

++batch_iter_;
for (; batch_iter_ != current_level_range_.end(); ++batch_iter_) {
struct FilePickerContext& fp_ctx = fp_ctx_array_[batch_iter_.index()];
Expand Down Expand Up @@ -803,6 +816,8 @@ class FilePickerMultiGet {
continue;
}
}
assert(start_index >= 0);
assert(start_index < static_cast<int32_t>(curr_file_level_->num_files));
fp_ctx.start_index_in_curr_level = start_index;
fp_ctx.curr_index_in_curr_level = start_index;
}
Expand Down

0 comments on commit 65e162b

Please sign in to comment.