From 1f5954147bd89ae7d60eb1d74e95b4eefc668fb8 Mon Sep 17 00:00:00 2001 From: sdong Date: Fri, 26 Feb 2016 17:13:39 -0800 Subject: [PATCH] Introduce Iterator::GetProperty() and replace Iterator::IsKeyPinned() Summary: Add Iterator::GetProperty(), a way for users to communicate with iterator, and turn Iterator::IsKeyPinned() with it. As a follow-up, I'll ask a property as the version number attached to the iterator Test Plan: Rerun existing tests and add a negative test case. Reviewers: yhchiang, andrewkr, kradhakrishnan, anthony, IslamAbdelRahman Reviewed By: IslamAbdelRahman Subscribers: leveldb, dhruba Differential Revision: https://reviews.facebook.net/D54783 --- HISTORY.md | 1 + db/db_iter.cc | 25 +++++++++++++++----- db/db_iter.h | 3 ++- db/db_test.cc | 48 +++++++++++++++++++++++++++++++++----- include/rocksdb/iterator.h | 19 ++++++++------- include/rocksdb/options.h | 5 ++-- table/iterator.cc | 11 +++++++++ 7 files changed, 89 insertions(+), 23 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index abf86a430..6561e520e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -2,6 +2,7 @@ ## Unreleased ### New Features * Add CompactionPri::kMinOverlappingRatio, a compaction picking mode friendly to write amplification. +* Deprecate Iterator::IsKeyPinned() and replace it with Iterator::GetProperty() with prop_name="rocksdb.iterator.is.key.pinned" ## 4.5.0 (2/5/2016) ### Public API Changes diff --git a/db/db_iter.cc b/db/db_iter.cc index afffc4dcc..c051a3928 100644 --- a/db/db_iter.cc +++ b/db/db_iter.cc @@ -136,9 +136,21 @@ class DBIter: public Iterator { } return s; } - virtual bool IsKeyPinned() const override { - assert(valid_); - return iter_pinned_ && saved_key_.IsKeyPinned(); + + virtual Status GetProperty(std::string prop_name, + std::string* prop) override { + if (prop == nullptr) { + return Status::InvalidArgument("prop is nullptr"); + } + if (prop_name == "rocksdb.iterator.is.key.pinned") { + if (valid_) { + *prop = (iter_pinned_ && saved_key_.IsKeyPinned()) ? "1" : "0"; + } else { + *prop = "Iterator is not valid."; + } + return Status::OK(); + } + return Status::InvalidArgument("Undentified property."); } virtual void Next() override; @@ -850,12 +862,13 @@ inline Slice ArenaWrappedDBIter::key() const { return db_iter_->key(); } inline Slice ArenaWrappedDBIter::value() const { return db_iter_->value(); } inline Status ArenaWrappedDBIter::status() const { return db_iter_->status(); } inline Status ArenaWrappedDBIter::PinData() { return db_iter_->PinData(); } +inline Status ArenaWrappedDBIter::GetProperty(std::string prop_name, + std::string* prop) { + return db_iter_->GetProperty(prop_name, prop); +} inline Status ArenaWrappedDBIter::ReleasePinnedData() { return db_iter_->ReleasePinnedData(); } -inline bool ArenaWrappedDBIter::IsKeyPinned() const { - return db_iter_->IsKeyPinned(); -} void ArenaWrappedDBIter::RegisterCleanup(CleanupFunction function, void* arg1, void* arg2) { db_iter_->RegisterCleanup(function, arg1, arg2); diff --git a/db/db_iter.h b/db/db_iter.h index 23bedb660..4060c6408 100644 --- a/db/db_iter.h +++ b/db/db_iter.h @@ -9,6 +9,7 @@ #pragma once #include +#include #include "rocksdb/db.h" #include "rocksdb/iterator.h" #include "db/dbformat.h" @@ -66,7 +67,7 @@ class ArenaWrappedDBIter : public Iterator { void RegisterCleanup(CleanupFunction function, void* arg1, void* arg2); virtual Status PinData(); virtual Status ReleasePinnedData(); - virtual bool IsKeyPinned() const override; + virtual Status GetProperty(std::string prop_name, std::string* prop) override; private: DBIter* db_iter_; diff --git a/db/db_test.cc b/db/db_test.cc index c9c2a6392..08e4edd93 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -632,6 +632,27 @@ TEST_F(DBTest, ReadFromPersistedTier) { } while (ChangeOptions(kSkipHashCuckoo)); } +TEST_F(DBTest, IteratorProperty) { + // The test needs to be changed if kPersistedTier is supported in iterator. + Options options = CurrentOptions(); + CreateAndReopenWithCF({"pikachu"}, options); + Put(1, "1", "2"); + ReadOptions ropt; + ropt.pin_data = false; + { + unique_ptr iter(db_->NewIterator(ropt, handles_[1])); + iter->SeekToFirst(); + std::string prop_value; + ASSERT_NOK(iter->GetProperty("non_existing.value", &prop_value)); + ASSERT_OK(iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("0", prop_value); + iter->Next(); + ASSERT_OK(iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("Iterator is not valid.", prop_value); + } + Close(); +} + TEST_F(DBTest, PersistedTierOnIterator) { // The test needs to be changed if kPersistedTier is supported in iterator. Options options = CurrentOptions(); @@ -9789,7 +9810,10 @@ TEST_F(DBTest, PinnedDataIteratorRandomized) { ASSERT_EQ(true_data.lower_bound(k), true_data.end()); continue; } - ASSERT_TRUE(iter->IsKeyPinned()); + std::string prop_value; + ASSERT_OK( + iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("1", prop_value); keys_slices.push_back(iter->key()); true_keys.push_back(true_data.lower_bound(k)->first); } @@ -9804,7 +9828,10 @@ TEST_F(DBTest, PinnedDataIteratorRandomized) { printf("Testing iterating forward on all keys\n"); std::vector all_keys; for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { - ASSERT_TRUE(iter->IsKeyPinned()); + std::string prop_value; + ASSERT_OK( + iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("1", prop_value); all_keys.push_back(iter->key()); } ASSERT_EQ(all_keys.size(), true_data.size()); @@ -9822,7 +9849,10 @@ TEST_F(DBTest, PinnedDataIteratorRandomized) { printf("Testing iterating backward on all keys\n"); std::vector all_keys; for (iter->SeekToLast(); iter->Valid(); iter->Prev()) { - ASSERT_TRUE(iter->IsKeyPinned()); + std::string prop_value; + ASSERT_OK( + iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("1", prop_value); all_keys.push_back(iter->key()); } ASSERT_EQ(all_keys.size(), true_data.size()); @@ -9893,7 +9923,9 @@ TEST_F(DBTest, PinnedDataIteratorMultipleFiles) { std::vector> results; for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { - ASSERT_TRUE(iter->IsKeyPinned()); + std::string prop_value; + ASSERT_OK(iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("1", prop_value); results.emplace_back(iter->key(), iter->value().ToString()); } @@ -9946,7 +9978,9 @@ TEST_F(DBTest, PinnedDataIteratorMergeOperator) { std::vector> results; for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { - ASSERT_TRUE(iter->IsKeyPinned()); + std::string prop_value; + ASSERT_OK(iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("1", prop_value); results.emplace_back(iter->key(), iter->value().ToString()); } @@ -10001,7 +10035,9 @@ TEST_F(DBTest, PinnedDataIteratorReadAfterUpdate) { std::vector> results; for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { - ASSERT_TRUE(iter->IsKeyPinned()); + std::string prop_value; + ASSERT_OK(iter->GetProperty("rocksdb.iterator.is.key.pinned", &prop_value)); + ASSERT_EQ("1", prop_value); results.emplace_back(iter->key(), iter->value().ToString()); } diff --git a/include/rocksdb/iterator.h b/include/rocksdb/iterator.h index ca08c35bf..4d9b9b89a 100644 --- a/include/rocksdb/iterator.h +++ b/include/rocksdb/iterator.h @@ -19,6 +19,7 @@ #ifndef STORAGE_ROCKSDB_INCLUDE_ITERATOR_H_ #define STORAGE_ROCKSDB_INCLUDE_ITERATOR_H_ +#include #include "rocksdb/slice.h" #include "rocksdb/status.h" @@ -95,14 +96,16 @@ class Iterator : public Cleanable { // satisfied without doing some IO, then this returns Status::Incomplete(). virtual Status status() const = 0; - // If true, this means that the Slice returned by key() is valid as long - // as the iterator is not deleted and ReleasePinnedData() is not called. - // - // IsKeyPinned() is guaranteed to always return true if - // - Iterator created with ReadOptions::pin_data = true - // - DB tables were created with BlockBasedTableOptions::use_delta_encoding - // set to false. - virtual bool IsKeyPinned() const { return false; } + // Property "rocksdb.iterator.is.key.pinned": + // If returning "1", this means that the Slice returned by key() is valid + // as long as the iterator is not deleted and ReleasePinnedData() is not + // called. + // It is guaranteed to always return "1" if + // - Iterator created with ReadOptions::pin_data = true + // - DB tables were created with + // BlockBasedTableOptions::use_delta_encoding + // set to false. + virtual Status GetProperty(std::string prop_name, std::string* prop); private: // No copying allowed diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index c0fe0b81a..9dbb5bbe8 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1399,8 +1399,9 @@ struct ReadOptions { // Keep the blocks loaded by the iterator pinned in memory as long as the // iterator is not deleted, If used when reading from tables created with - // BlockBasedTableOptions::use_delta_encoding = false, Iterator::IsKeyPinned() - // is guaranteed to return true. + // BlockBasedTableOptions::use_delta_encoding = false, + // Iterator's property "rocksdb.iterator.is.key.pinned" is guaranteed to + // return 1. // Default: false bool pin_data; diff --git a/table/iterator.cc b/table/iterator.cc index d99a8301f..0b53b41aa 100644 --- a/table/iterator.cc +++ b/table/iterator.cc @@ -46,6 +46,17 @@ void Cleanable::RegisterCleanup(CleanupFunction func, void* arg1, void* arg2) { c->arg2 = arg2; } +Status Iterator::GetProperty(std::string prop_name, std::string* prop) { + if (prop == nullptr) { + return Status::InvalidArgument("prop is nullptr"); + } + if (prop_name == "rocksdb.iterator.is.key.pinned") { + *prop = "0"; + return Status::OK(); + } + return Status::InvalidArgument("Undentified property."); +} + namespace { class EmptyIterator : public Iterator { public: