From 4d28a7d8ab9ad5d68b53d045808c4bbc5c1586e8 Mon Sep 17 00:00:00 2001 From: Venkatesh Radhakrishnan Date: Tue, 25 Aug 2015 13:40:58 -0700 Subject: [PATCH] Add a whitebox test for deleted file iterators. Summary: We have earlier added a feature to delete file iterators when the current key is over the iterate upper bound. We now add a whitebox test to check if the file iterators were actually deleted. Test Plan: Add check for a range which has deleted iterators. Reviewers: sdong Reviewed By: sdong Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D45321 --- db/db_tailing_iter_test.cc | 24 +++++++++++++++++++++++- db/forward_iterator.cc | 35 ++++++++++++++++++++++++++++++++++- db/forward_iterator.h | 1 + 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/db/db_tailing_iter_test.cc b/db/db_tailing_iter_test.cc index 478a48aa6..2d4953eb2 100644 --- a/db/db_tailing_iter_test.cc +++ b/db/db_tailing_iter_test.cc @@ -12,6 +12,7 @@ // which is a pity, it is a good test #if !(defined NDEBUG) || !defined(OS_WIN) +#include "db/forward_iterator.h" #include "port/stack_trace.h" #include "util/db_test_util.h" @@ -96,7 +97,10 @@ TEST_F(DBTestTailingIterator, TailingIteratorSeekToNext) { } ASSERT_TRUE(itern->Valid()); ASSERT_EQ(itern->key().compare(key), 0); + file_iters_deleted = false; } + rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks(); + rocksdb::SyncPoint::GetInstance()->DisableProcessing(); for (int i = 2 * num_records; i > 0; --i) { char buf1[32]; char buf2[32]; @@ -130,7 +134,18 @@ TEST_F(DBTestTailingIterator, TailingIteratorTrimSeekToNext) { std::unique_ptr itern(db_->NewIterator(read_options, handles_[1])); std::unique_ptr iterh(db_->NewIterator(read_options, handles_[1])); std::string value(1024, 'a'); - + bool file_iters_deleted = false; + rocksdb::SyncPoint::GetInstance()->SetCallBack( + "ForwardIterator::SeekInternal:Return", [&](void* arg) { + ForwardIterator* fiter = reinterpret_cast(arg); + ASSERT_TRUE(!file_iters_deleted || fiter->TEST_CheckDeletedIters()); + }); + rocksdb::SyncPoint::GetInstance()->SetCallBack( + "ForwardIterator::Next:Return", [&](void* arg) { + ForwardIterator* fiter = reinterpret_cast(arg); + ASSERT_TRUE(!file_iters_deleted || fiter->TEST_CheckDeletedIters()); + }); + rocksdb::SyncPoint::GetInstance()->EnableProcessing(); const int num_records = 1000; for (int i = 1; i < num_records; ++i) { char buf1[32]; @@ -148,6 +163,9 @@ TEST_F(DBTestTailingIterator, TailingIteratorTrimSeekToNext) { if (i % 100 == 99) { ASSERT_OK(Flush(1)); dbfull()->TEST_WaitForCompact(); + if (i == 299) { + file_iters_deleted = true; + } snprintf(buf4, sizeof(buf1), "00a0%016d", i * 5 / 2); Slice target(buf4, 20); iterh->Seek(target); @@ -156,8 +174,12 @@ TEST_F(DBTestTailingIterator, TailingIteratorTrimSeekToNext) { iterh->Next(); ASSERT_TRUE(iterh->Valid()); } + if (i == 299) { + file_iters_deleted = false; + } } + file_iters_deleted = ((i > 99) && (i % 400 != 399)); snprintf(buf2, sizeof(buf2), "00a0%016d", i * 5 - 2); Slice target(buf2, 20); iter->Seek(target); diff --git a/db/forward_iterator.cc b/db/forward_iterator.cc index c298f9f8b..2522f56e7 100644 --- a/db/forward_iterator.cc +++ b/db/forward_iterator.cc @@ -19,6 +19,7 @@ #include "rocksdb/slice_transform.h" #include "table/merger.h" #include "db/dbformat.h" +#include "util/sync_point.h" namespace rocksdb { @@ -389,6 +390,7 @@ void ForwardIterator::SeekInternal(const Slice& internal_key, } UpdateCurrent(); + TEST_SYNC_POINT_CALLBACK("ForwardIterator::SeekInternal:Return", this); } void ForwardIterator::Next() { @@ -443,6 +445,7 @@ void ForwardIterator::Next() { } } UpdateCurrent(); + TEST_SYNC_POINT_CALLBACK("ForwardIterator::Next:Return", this); } Slice ForwardIterator::key() const { @@ -474,6 +477,7 @@ void ForwardIterator::RebuildIterators(bool refresh_sv) { } mutable_iter_ = sv_->mem->NewIterator(read_options_, &arena_); sv_->imm->AddIterators(read_options_, &imm_iters_, &arena_); + has_iter_trimmed_for_upper_bound_ = false; const auto* vstorage = sv_->current->storage_info(); const auto& l0_files = vstorage->LevelFiles(0); @@ -499,6 +503,9 @@ void ForwardIterator::RebuildIterators(bool refresh_sv) { level_files[0]->smallest.user_key()) < 0))) { level_iters_.push_back(nullptr); + if (!level_files.empty()) { + has_iter_trimmed_for_upper_bound_ = true; + } } else { level_iters_.push_back( new LevelIterator(cfd_, read_options_, level_files)); @@ -507,7 +514,6 @@ void ForwardIterator::RebuildIterators(bool refresh_sv) { current_ = nullptr; is_prev_set_ = false; - has_iter_trimmed_for_upper_bound_ = false; } void ForwardIterator::ResetIncompleteIterators() { @@ -657,6 +663,33 @@ void ForwardIterator::DeleteCurrentIter() { } } +bool ForwardIterator::TEST_CheckDeletedIters() { + if (!has_iter_trimmed_for_upper_bound_) { + return false; + } + for (size_t i = 0; i < imm_iters_.size(); i++) { + auto& m = imm_iters_[i]; + if (!m) { + return true; + } + } + + const VersionStorageInfo* vstorage = sv_->current->storage_info(); + const std::vector& l0 = vstorage->LevelFiles(0); + for (uint32_t i = 0; i < l0.size(); ++i) { + if (!l0_iters_[i]) { + return true; + } + } + + for (int32_t level = 1; level < vstorage->num_levels(); ++level) { + if ((level_iters_[level - 1] == nullptr) && + (!vstorage->LevelFiles(level).empty())) { + return true; + } + } + return false; +} uint32_t ForwardIterator::FindFileInRange( const std::vector& files, const Slice& internal_key, uint32_t left, uint32_t right) { diff --git a/db/forward_iterator.h b/db/forward_iterator.h index 98fd9b114..72a23e757 100644 --- a/db/forward_iterator.h +++ b/db/forward_iterator.h @@ -70,6 +70,7 @@ class ForwardIterator : public Iterator { virtual Slice key() const override; virtual Slice value() const override; virtual Status status() const override; + bool TEST_CheckDeletedIters(); private: void Cleanup(bool release_sv);