os: KeyValueDB: re-implement (prefix) iter in terms of whole-space iter

In-a-nutshell-version: Create a whole-space iterator interface, and
implement the already existing, prefix-based iterator in terms of the
new whole-space iterator;

This patch introduces a significant change on the architecture of
KeyValueDB's iterator, although its interface remains the same.

Before this patch, KeyValueDB simply defined an interface for a
prefix-based interface, to be implemented by derivative classes. Being
constrained by a prefix-based approach to iterate over the store only makes
sense when we know which prefixes we want to iterate over, but for that we
must know about the prefixes beforehand. This approach didn't work when one
wanted to iterate over the whole key space, without any previous awareness
about the keys and their prefixes.

This patch introduces a new interface for a whole-space iterator, to be
implemented by derivative classes, which is prefix-independent. We also
define an abstract function to obtain this iterator, which must also be
implemented by the derivative class. With this interface in place, we are
then able to implement a prefix-dependent iterator in terms of the
whole-space iterator, which will be offered by the KeyValueDB class itself.

Furthermore, we implement these changes on LevelDBStore and KeyValueDBMemory,
the in-memory mock store, which leads to significant changes on both:

  * LevelDBStore
    - Substitute the previously existing LevelDBIteratorImpl, which
      followed a prefix-based iteration, for
      LevelDBWholeSpaceIteratorImpl, which now iterates over the whole
      key space of the store;

  * KeyValueDBMemory:
    - Substitute the previously existing MemIterator, which followed a
      prefix-based iteration, for WholeSpaceMemIterator, which now
      iterates over the whole key space of the in-memory mock store;
    - Change the in-memory mock store data structure. Previously, we
      used a map-of-maps, mapping prefixes to a key/value map; now we
      keep a single map, mapping (prefix,key) pairs to values.

Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>

diff --git a/src/os/KeyValueDB.h b/src/os/KeyValueDB.h
index 3b8b009990d9894f1602ea0636133631cf21e407..4be188f320ccebd5827733b950f1ed643d36bb98 100644 (file)
--- a/src/os/KeyValueDB.h
+++ b/src/os/KeyValueDB.h
@@ -8,6 +8,7 @@
 #include <map>
 #include <string>
 #include <tr1/memory>
+#include <boost/scoped_ptr.hpp>
 #include "ObjectMap.h"
 
 using std::string;
@@ -76,12 +77,14 @@ public:
     std::map<string, bufferlist> *out ///< [out] Key value retrieved
     ) = 0;
 
-  class IteratorImpl : public ObjectMap::ObjectMapIteratorImpl {
+  class WholeSpaceIteratorImpl {
   public:
     virtual int seek_to_first() = 0;
+    virtual int seek_to_first(const string &prefix) = 0;
     virtual int seek_to_last() = 0;
-    virtual int upper_bound(const string &after) = 0;
-    virtual int lower_bound(const string &to) = 0;
+    virtual int seek_to_last(const string &prefix) = 0;
+    virtual int upper_bound(const string &prefix, const string &after) = 0;
+    virtual int lower_bound(const string &prefix, const string &to) = 0;
     virtual bool valid() = 0;
     virtual int next() = 0;
     virtual int prev() = 0;
@@ -89,12 +92,73 @@ public:
     virtual pair<string,string> raw_key() = 0;
     virtual bufferlist value() = 0;
     virtual int status() = 0;
-    virtual ~IteratorImpl() {}
+    virtual ~WholeSpaceIteratorImpl() { }
+  };
+  typedef std::tr1::shared_ptr< WholeSpaceIteratorImpl > WholeSpaceIterator;
+
+  class IteratorImpl : public ObjectMap::ObjectMapIteratorImpl {
+    const string prefix;
+    WholeSpaceIterator generic_iter;
+  public:
+    IteratorImpl(const string &prefix, WholeSpaceIterator iter) :
+      prefix(prefix), generic_iter(iter) { }
+    virtual ~IteratorImpl() { }
+
+    int seek_to_first() {
+      return generic_iter->seek_to_first(prefix);
+    }
+    int seek_to_last() {
+      return generic_iter->seek_to_last(prefix);
+    }
+    int upper_bound(const string &after) {
+      return generic_iter->upper_bound(prefix, after);
+    }
+    int lower_bound(const string &to) {
+      return generic_iter->lower_bound(prefix, to);
+    }
+    bool valid() {
+      if (!generic_iter->valid())
+       return false;
+      pair<string,string> raw_key = generic_iter->raw_key();
+      return (raw_key.first == prefix);
+    }
+    int next() {
+      if (valid())
+       return generic_iter->next();
+      return status();
+    }
+    int prev() {
+      if (valid())
+       return generic_iter->prev();
+      return status();
+    }
+    string key() {
+      return generic_iter->key();
+    }
+    bufferlist value() {
+      return generic_iter->value();
+    }
+    int status() {
+      return generic_iter->status();
+    }
   };
+
   typedef std::tr1::shared_ptr< IteratorImpl > Iterator;
-  virtual Iterator get_iterator(const string &prefix) = 0;
+
+  WholeSpaceIterator get_iterator() {
+    return _get_iterator();
+  }
+
+  Iterator get_iterator(const string &prefix) {
+    return std::tr1::shared_ptr<IteratorImpl>(
+      new IteratorImpl(prefix, get_iterator())
+    );
+  }
 
   virtual ~KeyValueDB() {}
+
+protected:
+  virtual WholeSpaceIterator _get_iterator() = 0;
 };
 
 #endif

diff --git a/src/os/LevelDBStore.h b/src/os/LevelDBStore.h
index 570ff6d495d0f478db8931e20a13d9c5c52de8b7..a24a57fa84955ffaf43fafd794a295c0bb3810ad 100644 (file)
--- a/src/os/LevelDBStore.h
+++ b/src/os/LevelDBStore.h
@@ -1,4 +1,5 @@
 // -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
 #ifndef LEVEL_DB_STORE_H
 #define LEVEL_DB_STORE_H
 
@@ -73,42 +74,57 @@ public:
     std::map<string, bufferlist> *out
     );
 
-  class LevelDBIteratorImpl : public KeyValueDB::IteratorImpl {
+  class LevelDBWholeSpaceIteratorImpl :
+    public KeyValueDB::WholeSpaceIteratorImpl {
+  protected:
     boost::scoped_ptr<leveldb::Iterator> dbiter;
-    const string prefix;
   public:
-    LevelDBIteratorImpl(leveldb::Iterator *iter, const string &prefix) :
-      dbiter(iter), prefix(prefix) {}
+    LevelDBWholeSpaceIteratorImpl(leveldb::Iterator *iter) :
+      dbiter(iter) { }
+    virtual ~LevelDBWholeSpaceIteratorImpl() { }
+
     int seek_to_first() {
+      dbiter->SeekToFirst();
+      return dbiter->status().ok() ? 0 : -1;
+    }
+    int seek_to_first(const string &prefix) {
       leveldb::Slice slice_prefix(prefix);
       dbiter->Seek(slice_prefix);
       return dbiter->status().ok() ? 0 : -1;
     }
     int seek_to_last() {
+      dbiter->SeekToLast();
+      return dbiter->status().ok() ? 0 : -1;
+    }
+    int seek_to_last(const string &prefix) {
       string limit = past_prefix(prefix);
       leveldb::Slice slice_limit(limit);
-       dbiter->Seek(slice_limit);
+      dbiter->Seek(slice_limit);
+
       if (!dbiter->Valid()) {
-       dbiter->SeekToLast();
+        dbiter->SeekToLast();
       } else {
-       dbiter->Prev();
+        dbiter->Prev();
       }
       return dbiter->status().ok() ? 0 : -1;
     }
-    int upper_bound(const string &after) {
-      lower_bound(after);
-      if (valid() && key() == after)
-       next();
+    int upper_bound(const string &prefix, const string &after) {
+      lower_bound(prefix, after);
+      if (valid()) {
+       pair<string,string> key = raw_key();
+       if (key.first == prefix && key.second == after)
+         next();
+      }
       return dbiter->status().ok() ? 0 : -1;
     }
-    int lower_bound(const string &to) {
+    int lower_bound(const string &prefix, const string &to) {
       string bound = combine_strings(prefix, to);
       leveldb::Slice slice_bound(bound);
       dbiter->Seek(slice_bound);
       return dbiter->status().ok() ? 0 : -1;
     }
     bool valid() {
-      return dbiter->Valid() && in_prefix(prefix, dbiter->key());
+      return dbiter->Valid();
     }
     int next() {
       if (valid())
@@ -137,12 +153,6 @@ public:
       return dbiter->status().ok() ? 0 : -1;
     }
   };
-  Iterator get_iterator(const string &prefix) {
-    return std::tr1::shared_ptr<LevelDBIteratorImpl>(
-      new LevelDBIteratorImpl(
-       db->NewIterator(leveldb::ReadOptions()),
-       prefix));
-  }
 
 
   /// Utility
@@ -158,6 +168,15 @@ public:
     limit.push_back(1);
     return limit;
   }
+
+protected:
+  WholeSpaceIterator _get_iterator() {
+    return std::tr1::shared_ptr<KeyValueDB::WholeSpaceIteratorImpl>(
+      new LevelDBWholeSpaceIteratorImpl(
+       db->NewIterator(leveldb::ReadOptions())
+      )
+    );
+  }
 };
 
 #endif

diff --git a/src/test/ObjectMap/KeyValueDBMemory.cc b/src/test/ObjectMap/KeyValueDBMemory.cc
index 43ba1b2fcdeb3e0555a2f6b699916611ea0efb17..56aba4647efb669852d1ee03f44e35d9e91d95be 100644 (file)
--- a/src/test/ObjectMap/KeyValueDBMemory.cc
+++ b/src/test/ObjectMap/KeyValueDBMemory.cc
@@ -9,98 +9,140 @@
 
 using namespace std;
 
-class MemIterator : public KeyValueDB::IteratorImpl {
-  string prefix;
+/**
+ * Iterate over the whole key space of the in-memory store
+ *
+ * @note Removing keys from the store while iterating over the store key-space
+ *      may result in unspecified behavior.
+ *      If one wants to safely iterate over the store while updating the
+ *      store, one should instead use a snapshot iterator, which provides
+ *      strong read-consistency.
+ */
+class WholeSpaceMemIterator : public KeyValueDB::WholeSpaceIteratorImpl {
+protected:
   KeyValueDBMemory *db;
-
   bool ready;
-  map<string, bufferlist>::iterator iter;
+
+  map<pair<string,string>, bufferlist>::iterator it;
 
 public:
-  MemIterator(const string &prefix,
-             KeyValueDBMemory *db) :
-    prefix(prefix), db(db), ready(false) {}
+  WholeSpaceMemIterator(KeyValueDBMemory *db) : db(db), ready(false) { }
+  virtual ~WholeSpaceMemIterator() { }
 
   int seek_to_first() {
-    if (!db->db.count(prefix)) {
+    if (db->db.size() == 0) {
+      it = db->db.end();
+      ready = false;
+      return 0;
+    }
+    it = db->db.begin();
+    ready = true;
+    return 0;
+  }
+
+  int seek_to_first(const string &prefix) {
+    it = db->db.lower_bound(make_pair(prefix, ""));
+    if ((db->db.size() == 0) || (it == db->db.end())) {
+      it = db->db.end();
       ready = false;
       return 0;
     }
-    iter = db->db[prefix].begin();
     ready = true;
     return 0;
   }
 
   int seek_to_last() {
-    if (!db->db.count(prefix)) {
+    it = db->db.end();
+    if (db->db.size() == 0) {
       ready = false;
       return 0;
-    } else if (db->db[prefix].size() == 0) {
-      iter = db->db[prefix].end();
-    } else {
-      iter = --db->db[prefix].end();
     }
+    --it;
+    assert(it != db->db.end());
     ready = true;
     return 0;
   }
 
-  int lower_bound(const string &to) {
-    if (!db->db.count(prefix)) {
+  int seek_to_last(const string &prefix) {
+    string tmp(prefix);
+    tmp.append(1, (char) 0);
+    it = db->db.upper_bound(make_pair(tmp,""));
+
+    if ((db->db.size() == 0) || (it == db->db.end())) {
+      seek_to_last();
+    }
+    else {
+      ready = true;
+      prev();
+    }
+    return 0;
+  }
+
+  int lower_bound(const string &prefix, const string &to) {
+    it = db->db.lower_bound(make_pair(prefix,to));
+    if ((db->db.size() == 0) || (it == db->db.end())) {
+      it = db->db.end();
       ready = false;
       return 0;
     }
-    iter = db->db[prefix].lower_bound(to);
+
+    assert(it != db->db.end());
+
     ready = true;
     return 0;
   }
 
-  int upper_bound(const string &after) {
-    if (!db->db.count(prefix)) {
+  int upper_bound(const string &prefix, const string &after) {
+    it = db->db.upper_bound(make_pair(prefix,after));
+    if ((db->db.size() == 0) || (it == db->db.end())) {
+      it = db->db.end();
       ready = false;
       return 0;
     }
-    iter = db->db[prefix].upper_bound(after);
+    assert(it != db->db.end());
     ready = true;
     return 0;
   }
 
   bool valid() {
-    return ready && iter != db->db[prefix].end();
+    return ready && (it != db->db.end());
   }
 
   bool begin() {
-    return ready && iter == db->db[prefix].begin();
+    return ready && (it == db->db.begin());
   }
 
   int prev() {
-    if (valid() && iter != db->db[prefix].begin())
-      iter--;
+    if (!begin() && ready)
+      --it;
+    else
+      it = db->db.end();
     return 0;
   }
 
   int next() {
     if (valid())
-      iter++;
+      ++it;
     return 0;
   }
 
   string key() {
     if (valid())
-      return iter->first;
+      return (*it).first.second;
     else
       return "";
   }
 
   pair<string,string> raw_key() {
     if (valid())
-      return make_pair(prefix, key());
+      return (*it).first;
     else
-      return make_pair("","");
+      return make_pair("", "");
   }
 
   bufferlist value() {
     if (valid())
-      return iter->second;
+      return (*it).second;
     else
       return bufferlist();
   }
@@ -113,14 +155,15 @@ public:
 int KeyValueDBMemory::get(const string &prefix,
                          const std::set<string> &key,
                          map<string, bufferlist> *out) {
-  if (!db.count(prefix))
+  if (!exists_prefix(prefix))
     return 0;
 
   for (std::set<string>::const_iterator i = key.begin();
        i != key.end();
        ++i) {
-    if (db[prefix].count(*i))
-      (*out)[*i] = db[prefix][*i];
+    pair<string,string> k(prefix, *i);
+    if (db.count(k))
+      (*out)[*i] = db[k];
   }
   return 0;
 }
@@ -128,13 +171,13 @@ int KeyValueDBMemory::get(const string &prefix,
 int KeyValueDBMemory::get_keys(const string &prefix,
                               const std::set<string> &key,
                               std::set<string> *out) {
-  if (!db.count(prefix))
+  if (!exists_prefix(prefix))
     return 0;
 
   for (std::set<string>::const_iterator i = key.begin();
        i != key.end();
        ++i) {
-    if (db[prefix].count(*i))
+    if (db.count(make_pair(prefix, *i)))
       out->insert(*i);
   }
   return 0;
@@ -143,24 +186,35 @@ int KeyValueDBMemory::get_keys(const string &prefix,
 int KeyValueDBMemory::set(const string &prefix,
                          const string &key,
                          const bufferlist &bl) {
-  db[prefix][key] = bl;
+  db[make_pair(prefix,key)] = bl;
   return 0;
 }
 
 int KeyValueDBMemory::rmkey(const string &prefix,
                            const string &key) {
-  db[prefix].erase(key);
-  if (db[prefix].size() == 0)
-    db.erase(prefix);
-
+  db.erase(make_pair(prefix,key));
   return 0;
 }
 
 int KeyValueDBMemory::rmkeys_by_prefix(const string &prefix) {
-  db.erase(prefix);
+  map<std::pair<string,string>,bufferlist>::iterator i;
+  i = db.lower_bound(make_pair(prefix, ""));
+  if (i == db.end())
+    return 0;
+
+  while (i != db.end()) {
+    std::pair<string,string> key = (*i).first;
+    if (key.first != prefix)
+      break;
+
+    ++i;
+    rmkey(key.first, key.second);
+  }
   return 0;
 }
 
-KeyValueDB::Iterator KeyValueDBMemory::get_iterator(const string &prefix) {
-  return tr1::shared_ptr<IteratorImpl>(new MemIterator(prefix, this));
+KeyValueDB::WholeSpaceIterator KeyValueDBMemory::_get_iterator() {
+  return std::tr1::shared_ptr<KeyValueDB::WholeSpaceIteratorImpl>(
+    new WholeSpaceMemIterator(this)
+  );
 }

diff --git a/src/test/ObjectMap/KeyValueDBMemory.h b/src/test/ObjectMap/KeyValueDBMemory.h
index 5cee0527030435f95aaae4b262a1da4bc97eb2a6..c7bb28782383c753cb64f2b3fa45ee771a596449 100644 (file)
--- a/src/test/ObjectMap/KeyValueDBMemory.h
+++ b/src/test/ObjectMap/KeyValueDBMemory.h
@@ -13,7 +13,11 @@ using std::string;
 
 class KeyValueDBMemory : public KeyValueDB {
 public:
-  std::map<string, std::map<string, bufferlist> > db;
+  std::map<std::pair<string,string>,bufferlist> db;
+
+  KeyValueDBMemory() { }
+  KeyValueDBMemory(KeyValueDBMemory *db) : db(db->db) { }
+  virtual ~KeyValueDBMemory() { }
 
   int get(
     const string &prefix,
@@ -52,38 +56,34 @@ public:
 
     struct SetOp : public Context {
       KeyValueDBMemory *db;
-      string prefix;
-      string key;
+      std::pair<string,string> key;
       bufferlist value;
       SetOp(KeyValueDBMemory *db,
-           const string &prefix,
-           const string &key,
+           const std::pair<string,string> &key,
            const bufferlist &value)
-       : db(db), prefix(prefix), key(key), value(value) {}
+       : db(db), key(key), value(value) {}
       void finish(int r) {
-       db->set(prefix, key, value);
+       db->set(key.first, key.second, value);
       }
     };
 
     void set(const string &prefix, const string &k, const bufferlist& bl) {
-      on_commit.push_back(new SetOp(db, prefix, k, bl));
+      on_commit.push_back(new SetOp(db, std::make_pair(prefix, k), bl));
     }
 
     struct RmKeysOp : public Context {
       KeyValueDBMemory *db;
-      string prefix;
-      string key;
+      std::pair<string,string> key;
       RmKeysOp(KeyValueDBMemory *db,
-              const string &prefix,
-              const string &key)
-       : db(db), prefix(prefix), key(key) {}
+              const std::pair<string,string> &key)
+       : db(db), key(key) {}
       void finish(int r) {
-       db->rmkey(prefix, key);
+       db->rmkey(key.first, key.second);
       }
     };
 
     void rmkey(const string &prefix, const string &key) {
-      on_commit.push_back(new RmKeysOp(db, prefix, key));
+      on_commit.push_back(new RmKeysOp(db, std::make_pair(prefix, key)));
     }
 
     struct RmKeysByPrefixOp : public Context {
@@ -127,6 +127,15 @@ public:
     return static_cast<TransactionImpl_*>(trans.get())->complete();
   }
 
-  friend class MemIterator;
-  Iterator get_iterator(const string &prefix);
+private:
+  bool exists_prefix(const string &prefix) {
+    std::map<std::pair<string,string>,bufferlist>::iterator it;
+    it = db.lower_bound(std::make_pair(prefix, ""));
+    return ((it != db.end()) && ((*it).first.first == prefix));
+  }
+
+  friend class WholeSpaceMemIterator;
+
+protected:
+  WholeSpaceIterator _get_iterator();
 };