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

@@ -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

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);

db/db_iter.h

@@ -9,6 +9,7 @@
 
 #pragma once
 #include <stdint.h>
+#include <string>
 #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_;

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<Iterator> 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<Slice> 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<Slice> 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<std::pair<Slice, std::string>> 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<std::pair<Slice, std::string>> 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<std::pair<Slice, std::string>> 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());
   }
 

include/rocksdb/iterator.h

@@ -19,6 +19,7 @@
 #ifndef STORAGE_ROCKSDB_INCLUDE_ITERATOR_H_
 #define STORAGE_ROCKSDB_INCLUDE_ITERATOR_H_
 
+#include <string>
 #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

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;
 

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: