MB-24294: Stop couch-kvstore changing the file revision 24/78024/8
authorJim Walker <jim@couchbase.com>
Thu, 11 May 2017 15:38:35 +0000 (16:38 +0100)
committerDave Rigby <daver@couchbase.com>
Mon, 22 May 2017 08:33:24 +0000 (08:33 +0000)
There's a lot of code around the open paths which may open a different
file-revision to the one requested, which is the cause of MB-24294.

In summary we asked for 0.couch.6, which doesn't yet exist and the
checkNewRevNum function (now removed) would then look for 0.couch.*
files and depending on the async delete task's progress may find
0.couch.5 and direct saveDocs into that VB, which is about to be
deleted...

The main fix here is to remove checkNewRevNum, if we have a
dbFileRevMap value of n, we should open/create 0.couch.n and never
anything else.

This lead onto finding an issue where the RO instance of CouchKVStore
was relying on populating it's "copy" of the revision map via failing
to open 0.couch.0 and then looking for 0.couch.* and updating the map
from what it finds. So this is fixed by having a single dbFileRevMap
which is now referenced by the RW/RO pair.

Change-Id: I03dbb0c0a3e635397a12d85ea6092aa1312354b7
Reviewed-on: http://review.couchbase.org/78024
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
src/couch-kvstore/couch-kvstore.cc
src/couch-kvstore/couch-kvstore.h
src/kvshard.cc
src/kvstore.cc
src/kvstore.h
tests/module_tests/collections/evp_store_collections_test.cc
tests/module_tests/kvstore_test.cc

index f5ee558..62f2922 100644 (file)
@@ -55,8 +55,6 @@
 #include <kvstore.h>
 #include <platform/compress.h>
 
-static const int MAX_OPEN_DB_RETRY = 10;
-
 extern "C" {
     static int recordDbDumpC(Db *db, DocInfo *docinfo, void *ctx)
     {
@@ -273,21 +271,23 @@ CouchRequest::CouchRequest(const Item& it,
     dbDocInfo.content_meta = getContentMeta(it);
 }
 
-CouchKVStore::CouchKVStore(KVStoreConfig &config, bool read_only)
-    : CouchKVStore(config, *couchstore_get_default_file_ops(), read_only) {
-
+CouchKVStore::CouchKVStore(KVStoreConfig& config)
+    : CouchKVStore(config, *couchstore_get_default_file_ops()) {
 }
 
-CouchKVStore::CouchKVStore(KVStoreConfig &config, FileOpsInterface& ops,
-                           bool read_only)
-    : KVStore(config, read_only),
+CouchKVStore::CouchKVStore(KVStoreConfig& config,
+                           FileOpsInterface& ops,
+                           bool readOnly,
+                           std::vector<std::atomic<uint64_t>>& dbFileRevMap,
+                           size_t fileRevMapSize)
+    : KVStore(config, readOnly),
       dbname(config.getDBName()),
-      dbFileRevMap(configuration.getMaxVBuckets()),
+      dbFileRevMap(dbFileRevMap),
+      fileRevMap(fileRevMapSize),
       intransaction(false),
       scanCounter(0),
       logger(config.getLogger()),
-      base_ops(ops)
-{
+      base_ops(ops) {
     createDataDir(dbname);
     statCollectingFileOps = getCouchstoreStatsOps(st.fsStats, base_ops);
     statCollectingFileOpsCompaction = getCouchstoreStatsOps(
@@ -308,18 +308,26 @@ CouchKVStore::CouchKVStore(KVStoreConfig &config, FileOpsInterface& ops,
     initialize();
 }
 
-CouchKVStore::CouchKVStore(const CouchKVStore &copyFrom)
+CouchKVStore::CouchKVStore(KVStoreConfig& config, FileOpsInterface& ops)
+    : CouchKVStore(config,
+                   ops,
+                   false /*readonly*/,
+                   fileRevMap,
+                   config.getMaxVBuckets()) {
+}
+
+CouchKVStore::CouchKVStore(const CouchKVStore& copyFrom)
     : KVStore(copyFrom),
       dbname(copyFrom.dbname),
-      dbFileRevMap(copyFrom.dbFileRevMap.size()),
+      dbFileRevMap(copyFrom.dbFileRevMap),
+      fileRevMap(copyFrom.fileRevMap.size()),
       numDbFiles(copyFrom.numDbFiles),
       intransaction(false),
       logger(copyFrom.logger),
-      base_ops(copyFrom.base_ops)
-{
+      base_ops(copyFrom.base_ops) {
     // std::atomic needs to be manually copied (not copy constructor)
-    for (size_t ii = 0; ii < dbFileRevMap.size(); ii++) {
-        dbFileRevMap[ii].store(copyFrom.dbFileRevMap[ii].load());
+    for (size_t ii = 0; ii < fileRevMap.size(); ii++) {
+        fileRevMap[ii].store(copyFrom.fileRevMap[ii].load());
     }
     createDataDir(dbname);
     statCollectingFileOps = getCouchstoreStatsOps(st.fsStats, base_ops);
@@ -327,6 +335,24 @@ CouchKVStore::CouchKVStore(const CouchKVStore &copyFrom)
         st.fsStatsCompaction, base_ops);
 }
 
+/**
+ * Make a read-only CouchKVStore from this object
+ */
+std::unique_ptr<CouchKVStore> CouchKVStore::makeReadOnlyStore() {
+    // Not using make_unique due to the private constructor we're calling
+    return std::unique_ptr<CouchKVStore>(
+            new CouchKVStore(configuration, fileRevMap));
+}
+
+CouchKVStore::CouchKVStore(KVStoreConfig& config,
+                           std::vector<std::atomic<uint64_t>>& dbFileRevMap)
+    : CouchKVStore(config,
+                   *couchstore_get_default_file_ops(),
+                   true /*readonly*/,
+                   dbFileRevMap,
+                   0) {
+}
+
 void CouchKVStore::initialize() {
     std::vector<uint16_t> vbids;
     std::vector<std::string> files;
@@ -661,9 +687,8 @@ void CouchKVStore::getPersistedStats(std::map<std::string,
 static std::string getDBFileName(const std::string &dbname,
                                  uint16_t vbid,
                                  uint64_t rev) {
-    std::stringstream ss;
-    ss << dbname << "/" << vbid << ".couch." << rev;
-    return ss.str();
+    return dbname + "/" + std::to_string(vbid) + ".couch." +
+           std::to_string(rev);
 }
 
 static int edit_docinfo_hook(DocInfo **info, const sized_buf *item) {
@@ -1425,8 +1450,8 @@ void CouchKVStore::updateDbFileMap(uint16_t vbucketId, uint64_t newFileRev) {
 
 couchstore_error_t CouchKVStore::openDB(uint16_t vbucketId,
                                         uint64_t fileRev,
-                                        Db **db,
-                                        uint64_t options,
+                                        Db** db,
+                                        couchstore_open_flags options,
                                         FileOpsInterface* ops) {
     std::string dbFileName = getDBFileName(dbname, vbucketId, fileRev);
 
@@ -1434,7 +1459,6 @@ couchstore_error_t CouchKVStore::openDB(uint16_t vbucketId,
         ops = statCollectingFileOps.get();
     }
 
-    uint64_t newRevNum = fileRev;
     couchstore_error_t errorCode = COUCHSTORE_SUCCESS;
 
     /**
@@ -1447,88 +1471,25 @@ couchstore_error_t CouchKVStore::openDB(uint16_t vbucketId,
         options |= COUCHSTORE_OPEN_FLAG_UNBUFFERED;
     }
 
-    if (options & COUCHSTORE_OPEN_FLAG_CREATE) {
-        // first try to open the requested file without the
-        // create option in case it does already exist
-        errorCode = couchstore_open_db_ex(dbFileName.c_str(), 0, ops, db);
-        if (errorCode != COUCHSTORE_SUCCESS) {
-            // open_db failed but still check if the file exists
-            newRevNum = checkNewRevNum(dbFileName);
-            bool fileExists = (newRevNum) ? true : false;
-            if (fileExists) {
-                errorCode = openDB_retry(dbFileName, 0, ops, db, &newRevNum);
-            } else {
-                // requested file doesn't seem to exist, just create one
-                errorCode = couchstore_open_db_ex(
-                        dbFileName.c_str(), options, ops, db);
-                if (errorCode == COUCHSTORE_SUCCESS) {
-                    logger.log(EXTENSION_LOG_INFO,
-                               "CouchKVStore::openDB: created new couch db "
-                               "file, name:%s "
-                               "rev:%" PRIu64,
-                               dbFileName.c_str(),
-                               fileRev);
-                } else {
-                    logger.log(EXTENSION_LOG_WARNING,
-                               "CouchKVStore::openDB: couchstore_open_db_ex"
-                               " error:%s, name:%s, rev:%" PRIu64
-                               ", reset:false",
-                               couchstore_strerror(errorCode),
-                               dbFileName.c_str(),
-                               fileRev);
-                }
-            }
-        }
-    } else {
-        errorCode = openDB_retry(dbFileName, options, ops, db, &newRevNum);
-    }
+    errorCode = couchstore_open_db_ex(dbFileName.c_str(), options, ops, db);
 
     /* update command statistics */
     st.numOpen++;
     if (errorCode) {
         st.numOpenFailure++;
-        logger.log(EXTENSION_LOG_WARNING, "CouchKVStore::openDB: error:%s [%s],"
-                   " name:%s, option:%" PRIX64 ", rev:%" PRIu64,
+        logger.log(EXTENSION_LOG_WARNING,
+                   "CouchKVStore::openDB: error:%s [%s],"
+                   " name:%s, option:%" PRIX64 ", fileRev:%" PRIu64,
                    couchstore_strerror(errorCode),
-                   cb_strerror().c_str(), dbFileName.c_str(), options,
-                   ((newRevNum > fileRev) ? newRevNum : fileRev));
+                   cb_strerror().c_str(),
+                   dbFileName.c_str(),
+                   options,
+                   fileRev);
     }
 
     return errorCode;
 }
 
-couchstore_error_t CouchKVStore::openDB_retry(std::string &dbfile,
-                                              uint64_t options,
-                                              FileOpsInterface *ops,
-                                              Db** db, uint64_t *newFileRev) {
-    int retry = 0;
-    couchstore_error_t errCode = COUCHSTORE_SUCCESS;
-
-    while (retry < MAX_OPEN_DB_RETRY) {
-        errCode = couchstore_open_db_ex(dbfile.c_str(), options, ops, db);
-        if (errCode == COUCHSTORE_SUCCESS) {
-            return errCode;
-        }
-        logger.log(EXTENSION_LOG_NOTICE,
-                   "CouchKVStore::openDB_retry: couchstore_open_db "
-                   "error:%s [%s], name:%s, options:%" PRIX64,
-                   couchstore_strerror(errCode), cb_strerror().c_str(),
-                   dbfile.c_str(), options);
-        *newFileRev = checkNewRevNum(dbfile);
-        ++retry;
-        if (retry == MAX_OPEN_DB_RETRY - 1 &&
-            !(options & COUCHSTORE_OPEN_FLAG_CREATE) &&
-            !(options & COUCHSTORE_OPEN_FLAG_RDONLY) &&
-            errCode == COUCHSTORE_ERROR_NO_SUCH_FILE) {
-            // Last retry:
-            // if file still doesn't exist, attempt to create it
-            // if we are allowed (i.e., not read-only mode).
-            options |= COUCHSTORE_OPEN_FLAG_CREATE;
-        }
-    }
-    return errCode;
-}
-
 void CouchKVStore::populateFileNameMap(std::vector<std::string> &filenames,
                                        std::vector<uint16_t> *vbids) {
     std::vector<std::string>::iterator fileItr;
@@ -1921,7 +1882,8 @@ couchstore_error_t CouchKVStore::saveDocs(uint16_t vbid,
     }
 
     DbHolder db(this);
-    errCode = openDB(vbid, fileRev, db.getDbAddress(), 0);
+    errCode = openDB(
+            vbid, fileRev, db.getDbAddress(), COUCHSTORE_OPEN_FLAG_CREATE);
     if (errCode != COUCHSTORE_SUCCESS) {
         logger.log(EXTENSION_LOG_WARNING,
                    "CouchKVStore::saveDocs: openDB error:%s, vb:%" PRIu16
@@ -2750,8 +2712,10 @@ bool CouchKVStore::persistCollectionsManifestItem(uint16_t vbid,
     DbHolder db(this);
 
     // openDB logs error details
-    couchstore_error_t errCode =
-            openDB(vbid, 0, db.getDbAddress(), COUCHSTORE_OPEN_FLAG_CREATE);
+    couchstore_error_t errCode = openDB(vbid,
+                                        dbFileRevMap[vbid],
+                                        db.getDbAddress(),
+                                        COUCHSTORE_OPEN_FLAG_CREATE);
     if (errCode != COUCHSTORE_SUCCESS) {
         return false;
     }
@@ -2775,8 +2739,10 @@ std::string CouchKVStore::getCollectionsManifest(uint16_t vbid) {
     DbHolder db(this);
 
     // openDB logs error details
-    couchstore_error_t errCode =
-            openDB(vbid, 0, db.getDbAddress(), COUCHSTORE_OPEN_FLAG_CREATE);
+    couchstore_error_t errCode = openDB(vbid,
+                                        dbFileRevMap[vbid],
+                                        db.getDbAddress(),
+                                        COUCHSTORE_OPEN_FLAG_CREATE);
     if (errCode != COUCHSTORE_SUCCESS) {
         return {};
     }
index 0aad180..42f36b6 100644 (file)
@@ -151,22 +151,19 @@ class CouchKVStore : public KVStore
 {
 public:
     /**
-     * Constructor
+     * Constructor - creates a read/write CouchKVStore
      *
      * @param config    Configuration information
-     * @param read_only flag indicating if this kvstore instance is for read-only operations
      */
-    CouchKVStore(KVStoreConfig &config, bool read_only = false);
+    CouchKVStore(KVStoreConfig& config);
 
     /**
      * Alternate constructor for injecting base FileOps
      *
      * @param config    Configuration information
      * @param ops       Couchstore FileOps implementation to be used
-     * @param read_only flag indicating if this kvstore instance is for read-only operations
      */
-    CouchKVStore(KVStoreConfig &config, FileOpsInterface& ops,
-                 bool read_only = false);
+    CouchKVStore(KVStoreConfig& config, FileOpsInterface& ops);
 
     /**
      * Copy constructor
@@ -180,6 +177,14 @@ public:
      */
     ~CouchKVStore();
 
+    /**
+     * A read only CouchKVStore can only be created by a RW store. They should
+     * be created in pairs as they share some data.
+     *
+     * @return a unique_ptr holding a RO 'sibling' to this object.
+     */
+    std::unique_ptr<CouchKVStore> makeReadOnlyStore();
+
     void initialize();
 
     /**
@@ -481,11 +486,8 @@ protected:
     couchstore_error_t openDB(uint16_t vbucketId,
                               uint64_t fileRev,
                               Db** db,
-                              uint64_t options,
+                              couchstore_open_flags options,
                               FileOpsInterface* ops = nullptr);
-    couchstore_error_t openDB_retry(std::string &dbfile, uint64_t options,
-                                    FileOpsInterface *ops,
-                                    Db **db, uint64_t *newFileRev);
 
     /**
      * save the Documents held in docs to the file associated with vbid/rev
@@ -559,9 +561,17 @@ protected:
     const std::string dbname;
 
     /**
-     * Per-vbucket file revision atomic to ensure writer threads see increments
+     * Per-vbucket file revision atomic to ensure writer threads see increments.
+     * This is a reference of the real vector which there should be one per
+     * RW/RO pair.
+     */
+    std::vector<std::atomic<uint64_t>>& dbFileRevMap;
+
+    /**
+     * The RW couch-kvstore owns the fileRevMap and passes a reference to it's
+     * RO sibling.
      */
-    std::vector<std::atomic<uint64_t>> dbFileRevMap;
+    std::vector<std::atomic<uint64_t>> fileRevMap;
 
     uint16_t numDbFiles;
     std::vector<CouchRequest *> pendingReqsQ;
@@ -603,6 +613,38 @@ protected:
     FileOpsInterface& base_ops;
 
 private:
+    /**
+     * Construct the store, this constructor does the object initialisation and
+     * is used by the public read/write constructors and the private read-only
+     * constructor.
+     *
+     * @param config configuration data for the store
+     * @param ops the file ops to use
+     * @param readOnly true if the store can only do read functionality
+     * @param dbFileRevMap a reference to the map (which should be data owned by
+     *        the RW store).
+     * @param fileRevMapSize how big to make the fileRevMap. E.g. when the
+     *        read-only constructor is called, it doesn't need to resize the map
+     *        as it will use a reference to the RW store's map, so 0 would be
+     *        passed.
+     */
+    CouchKVStore(KVStoreConfig& config,
+                 FileOpsInterface& ops,
+                 bool readOnly,
+                 std::vector<std::atomic<uint64_t>>& dbFileRevMap,
+                 size_t fileRevMapSize);
+
+    /**
+     * Construct a read-only store - private as should be called via
+     * CouchKVStore::makeReadOnlyStore
+     *
+     * @param config configuration data for the store
+     * @param dbFileRevMap a reference to the map (which should be data owned by
+     *        the RW store).
+     */
+    CouchKVStore(KVStoreConfig& config,
+                 std::vector<std::atomic<uint64_t>>& dbFileRevMap);
+
     class DbHolder {
     public:
         DbHolder(CouchKVStore* kvs) : kvstore(kvs), db(nullptr) {}
index e0e8388..73f88e8 100644 (file)
@@ -35,10 +35,9 @@ KVShard::KVShard(uint16_t id, KVBucket& kvBucket)
     const std::string backend = kvConfig.getBackend();
 
     if (backend == "couchdb") {
-        rwStore.reset(KVStoreFactory::create(kvConfig, false));
-        roStore.reset(KVStoreFactory::create(kvConfig, true));
-    } else if (backend == "forestdb") {
-        rwStore.reset(KVStoreFactory::create(kvConfig));
+        auto stores = KVStoreFactory::create(kvConfig);
+        rwStore = std::move(stores.rw);
+        roStore = std::move(stores.ro);
     } else {
         throw std::logic_error(
                 "KVShard::KVShard: "
index acb4287..778a6c5 100644 (file)
@@ -68,20 +68,17 @@ KVStoreConfig& KVStoreConfig::setBuffered(bool _buffered) {
     return *this;
 }
 
-KVStore *KVStoreFactory::create(KVStoreConfig &config, bool read_only) {
-    KVStore *ret = NULL;
-    std::string backend = config.getBackend();
-    if (backend.compare("couchdb") == 0) {
-        ret = new CouchKVStore(config, read_only);
-#ifdef EP_USE_FORESTDB
-    } else if (backend.compare("forestdb") == 0) {
-        ret = new ForestKVStore(config);
-#endif
+KVStoreRWRO KVStoreFactory::create(KVStoreConfig& config) {
+    if (config.getBackend().compare("couchdb") == 0) {
+        auto rw = std::make_unique<CouchKVStore>(config);
+        auto ro = rw->makeReadOnlyStore();
+        return {rw.release(), ro.release()};
     } else {
-        LOG(EXTENSION_LOG_WARNING, "Unknown backend: [%s]", backend.c_str());
+        throw std::invalid_argument("KVStoreFactory::create unknown backend:" +
+                                    config.getBackend());
     }
 
-    return ret;
+    return {};
 }
 
 void KVStore::createDataDir(const std::string& dbname) {
index bbd3a10..7643234 100644 (file)
@@ -581,7 +581,7 @@ public:
         return dbname;
     }
 
-    std::string getBackend() const {
+    const std::string& getBackend() const {
         return backend;
     }
 
@@ -1013,19 +1013,31 @@ protected:
 };
 
 /**
+ * Structure holding the read/write and read only instances of the KVStore.
+ * They could be the same underlying object, or different.
+ */
+struct KVStoreRWRO {
+    KVStoreRWRO() /*rw/ro default init is ok*/ {
+    }
+    KVStoreRWRO(KVStore* rw, KVStore* ro) : rw(rw), ro(ro) {
+    }
+
+    std::unique_ptr<KVStore> rw;
+    std::unique_ptr<KVStore> ro;
+};
+
+/**
  * The KVStoreFactory creates the correct KVStore instance(s) when
  * needed by EPStore.
  */
 class KVStoreFactory {
 public:
-
     /**
-     * Create a KVStore with the given type.
+     * Create a KVStore using the type found in the config
      *
-     * @param config    engine configuration
-     * @param read_only true if the kvstore instance is for read operations only
+     * @param config engine configuration
      */
-    static KVStore *create(KVStoreConfig &config, bool read_only = false);
+    static KVStoreRWRO create(KVStoreConfig& config);
 };
 
 /**
index 78f17f3..8e6ae79 100644 (file)
@@ -147,10 +147,6 @@ TEST_F(CollectionsTest, collections_basic) {
 class CollectionsFlushTest : public CollectionsTest {
 public:
     void SetUp() override {
-        kvsc = std::make_unique<KVStoreConfig>(
-                1024, 4, test_dbname, "couchdb", 0, true /*persistnamespace*/);
-        kvs = std::unique_ptr<KVStore>(KVStoreFactory::create(*kvsc));
-
         CollectionsTest::SetUp();
     }
 
@@ -192,9 +188,6 @@ private:
      */
     static bool cannotWrite(const std::string& jsonManifest,
                             const std::string& collection);
-
-    std::unique_ptr<KVStore> kvs;
-    std::unique_ptr<KVStoreConfig> kvsc;
 };
 
 void CollectionsFlushTest::storeItems(const std::string& collection,
@@ -234,7 +227,8 @@ std::string CollectionsFlushTest::completeDeletionAndFlush(
 }
 
 std::string CollectionsFlushTest::getManifest() {
-    return kvs->getCollectionsManifest(vbid);
+    VBucketPtr vb = store->getVBucket(vbid);
+    return vb->getShard()->getRWUnderlying()->getCollectionsManifest(vbid);
 }
 
 bool CollectionsFlushTest::canWrite(const std::string& jsonManifest,
index 7123bad..475f694 100644 (file)
@@ -157,6 +157,8 @@ protected:
 // Initializes a KVStore
 static void initialize_kv_store(KVStore* kvstore) {
     std::string failoverLog("");
+    // simulate the setVbState by incrementing the rev
+    kvstore->incrementRevision(0);
     vbucket_state state(vbucket_state_active, 0, 0, 0, 0, 0, 0, 0, failoverLog);
     // simulate the setVbState by incrementing the rev
     kvstore->incrementRevision(0);
@@ -166,9 +168,9 @@ static void initialize_kv_store(KVStore* kvstore) {
 
 // Creates and initializes a KVStore with the given config
 static std::unique_ptr<KVStore> setup_kv_store(KVStoreConfig& config) {
-    auto kvstore = std::unique_ptr<KVStore>(KVStoreFactory::create(config));
-    initialize_kv_store(kvstore.get());
-    return kvstore;
+    auto kvstore = KVStoreFactory::create(config);
+    initialize_kv_store(kvstore.rw.get());
+    return std::move(kvstore.rw);
 }
 
 /* Test callback for stats handling.
@@ -346,28 +348,24 @@ TEST_F(CouchKVStoreTest, CompactStatsTest) {
 TEST_F(CouchKVStoreTest, MB_17517MaxCasOfMinus1) {
     KVStoreConfig config(
             1024, 4, data_dir, "couchdb", 0, false /*persistnamespace*/);
-    KVStore* kvstore = KVStoreFactory::create(config);
-    ASSERT_NE(nullptr, kvstore);
+    auto kvstore = KVStoreFactory::create(config);
+    ASSERT_NE(nullptr, kvstore.rw);
 
     // Activate vBucket.
     std::string failoverLog("[]");
     vbucket_state state(vbucket_state_active, /*ckid*/0, /*maxDelSeqNum*/0,
                         /*highSeqno*/0, /*purgeSeqno*/0, /*lastSnapStart*/0,
                         /*lastSnapEnd*/0, /*maxCas*/-1, failoverLog);
-    EXPECT_TRUE(kvstore->snapshotVBucket(/*vbid*/0, state,
-                                         VBStatePersist::VBSTATE_PERSIST_WITHOUT_COMMIT));
-    EXPECT_EQ(~0ull, kvstore->listPersistedVbuckets()[0]->maxCas);
+    EXPECT_TRUE(kvstore.rw->snapshotVBucket(
+            /*vbid*/ 0, state, VBStatePersist::VBSTATE_PERSIST_WITHOUT_COMMIT));
+    EXPECT_EQ(~0ull, kvstore.rw->listPersistedVbuckets()[0]->maxCas);
 
     // Close the file, then re-open.
-    delete kvstore;
     kvstore = KVStoreFactory::create(config);
-    EXPECT_NE(nullptr, kvstore);
+    EXPECT_NE(nullptr, kvstore.rw);
 
     // Check that our max CAS was repaired on startup.
-    EXPECT_EQ(0u, kvstore->listPersistedVbuckets()[0]->maxCas);
-
-    // Cleanup
-    delete kvstore;
+    EXPECT_EQ(0u, kvstore.rw->listPersistedVbuckets()[0]->maxCas);
 }
 
 // Regression test for MB-19430 - ensure that an attempt to get the
@@ -378,14 +376,11 @@ TEST_F(CouchKVStoreTest, MB_18580_ENOENT) {
             1024, 4, data_dir, "couchdb", 0, false /*persistnamespace*/);
     // Create a read-only kvstore (which disables item count caching), then
     // attempt to get the count from a non-existent vbucket.
-    KVStore* kvstore = KVStoreFactory::create(config, /*readOnly*/true);
-    ASSERT_NE(nullptr, kvstore);
+    auto kvstore = KVStoreFactory::create(config);
+    ASSERT_NE(nullptr, kvstore.ro);
 
     // Expect to get a system_error (ENOENT)
-    EXPECT_THROW(kvstore->getDbFileInfo(0), std::system_error);
-
-    // Cleanup
-    delete kvstore;
+    EXPECT_THROW(kvstore.ro->getDbFileInfo(0), std::system_error);
 }
 
 /**
@@ -492,7 +487,7 @@ public:
                          .setLogger(logger)
                          .setBuffered(false)) {
         cb::io::rmrf(data_dir.c_str());
-        kvstore.reset(new CouchKVStore(config, ops, false));
+        kvstore.reset(new CouchKVStore(config, ops));
         initialize_kv_store(kvstore.get());
     }
     ~CouchKVStoreErrorInjectionTest() {
@@ -1117,8 +1112,8 @@ public:
 
 class MockCouchKVStore : public CouchKVStore {
 public:
-    MockCouchKVStore(KVStoreConfig& config)
-        : CouchKVStore(config, false) {}
+    MockCouchKVStore(KVStoreConfig& config) : CouchKVStore(config) {
+    }
 
     // Mocks original code but returns the IORequest for fuzzing
     MockCouchRequest* setAndReturnRequest(const Item &itm, Callback<mutation_result> &cb) {
@@ -1164,6 +1159,9 @@ public:
         kvstore.reset(new MockCouchKVStore(config));
         StatsCallback sc;
         std::string failoverLog("");
+        // simulate a setVBState - increment the rev and then persist the
+        // state
+        kvstore->incrementRevision(0);
         vbucket_state state(vbucket_state_active, 0, 0, 0, 0, 0, 0, 0,
                             failoverLog);
         // simulate a setVBState - increment the dbFile revision