MB-48094: Add new KVStoreIface::getCollectionStats() with vbid arg

Add new version of KVStoreIface::getCollectionStats() that takes a Vbid
as an arg rather than a KVFileHandle, so that there isn't requirement
to have created a KVFileHandle first. As this isn't necessary for
MagmaKVStore in all situations and it's expensive to create a
MagmaKVFileHandle.

This patch also refactors the majority of calls to getCollectionStats()
to use the Vbid. Rather than the KVFileHandle version.

Change-Id: I822b035827c25fb41bf044da0a00b2bb1bbe87eb
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/161241
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Reviewed-by: Ben Huddleston <ben.huddleston@couchbase.com>

engines/ep/src/ep_vb.cc

@@ -1079,13 +1079,12 @@ void EPVBucket::collectionsRolledBack(KVBucket& bucket) {
 
     manifest = std::make_unique<Collections::VB::Manifest>(
             bucket.getSharedCollectionsManager(), persistedManifest);
-    auto kvstoreContext = kvstore.makeFileHandle(getId());
     auto wh = manifest->wlock();
     // For each collection in the VB, reload the stats to the point before
     // the rollback seqno
     for (auto& collection : wh) {
         auto [status, stats] =
-                kvstore.getCollectionStats(*kvstoreContext, collection.first);
+                kvstore.getCollectionStats(getId(), collection.first);
         if (status == KVStore::GetCollectionStatsStatus::Failed) {
             EP_LOG_WARN(
                     "EPVBucket::collectionsRolledBack(): getCollectionStats() "

engines/ep/src/kvstore/couch-kvstore/couch-kvstore.cc

@@ -1304,7 +1304,8 @@ couchstore_error_t CouchKVStore::replayPreCopyLocalDoc(
         // This is a collection statistics doc, obtain collection-ID
         auto cid = getCollectionIdFromStatsDocId(documentId);
         // and load the stats
-        auto [status, currentStats] = getCollectionStats(target, cid);
+        auto [status, currentStats] =
+                getCollectionStats(target, getCollectionStatsLocalDocId(cid));
         if (status == GetCollectionStatsStatus::Failed) {
             logger.warn(
                     "CouchKVStore::replayPreCopyLocalDoc: failed loading stats "
@@ -1809,7 +1810,8 @@ couchstore_error_t CouchKVStore::maybePatchMetaData(
         // that in the case of a first compaction after upgrade from pre 7.0
         // we get the correct disk (because we use the dbinfo.space_used). The
         // target file could return a value too small in that case (MB-45917)
-        auto [status, currentStats] = getCollectionStats(source, cid);
+        auto [status, currentStats] =
+                getCollectionStats(source, getCollectionStatsLocalDocId(cid));
         if (status == KVStore::GetCollectionStatsStatus::Failed) {
             logger.warn(
                     "CouchKVStore::maybePatchMetaData: Failed to load "
@@ -3383,11 +3385,12 @@ std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
 CouchKVStore::getCollectionStats(const KVFileHandle& kvFileHandle,
                                  CollectionID collection) const {
     const auto& db = static_cast<const CouchKVFileHandle&>(kvFileHandle);
-    return getCollectionStats(*db.getDb(), collection);
+    return getCollectionStats(*db.getDb(),
+                              getCollectionStatsLocalDocId(collection));
 }
 
 std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
-CouchKVStore::getCollectionStats(Db& db, CollectionID collection) const {
+CouchKVStore::getCollectionStats(Vbid vbid, CollectionID collection) const {
     if (!collection.isUserCollection()) {
         logger.warn(
                 "CouchKVStore::getCollectionStats stats are not available for "
@@ -3396,7 +3399,19 @@ CouchKVStore::getCollectionStats(Db& db, CollectionID collection) const {
         return {KVStore::GetCollectionStatsStatus::Failed,
                 Collections::VB::PersistedStats{}};
     }
-    return getCollectionStats(db, getCollectionStatsLocalDocId(collection));
+    auto fileHandle = makeFileHandle(vbid);
+    if (!fileHandle) {
+        logger.warn(
+                "CouchKVStore::getCollectionStats unable to get file handle "
+                "for "
+                "{}",
+                vbid);
+        return {KVStore::GetCollectionStatsStatus::Failed,
+                Collections::VB::PersistedStats{}};
+    }
+    const auto& db = static_cast<const CouchKVFileHandle&>(*fileHandle);
+    return getCollectionStats(*db.getDb(),
+                              getCollectionStatsLocalDocId(collection));
 }
 
 std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>

engines/ep/src/kvstore/couch-kvstore/couch-kvstore.h

@@ -340,6 +340,9 @@ class CouchKVStore : public KVStore
     getCollectionStats(const KVFileHandle& kvFileHandle,
                        CollectionID collection) const override;
 
+    std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>
+    getCollectionStats(Vbid vbid, CollectionID collection) const override;
+
     /**
      * prepareToCreate will increment the revision number of the vbucket, but is
      * a no-op if readOnly()
@@ -586,9 +589,6 @@ class CouchKVStore : public KVStore
     void deleteCollectionStats(CouchKVStoreTransactionContext& txnCtx,
                                CollectionID cid);
 
-    /// Get the collection stats for the given collection
-    std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>
-    getCollectionStats(Db& db, CollectionID collection) const;
 
     /// Get the collection stats from the local doc named statDocName
     std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>

engines/ep/src/kvstore/kvstore_iface.h

@@ -507,6 +507,21 @@ class KVStoreIface {
     getCollectionStats(const KVFileHandle& kvFileHandle,
                        CollectionID collection) const = 0;
 
+    /**
+     * Retrieve the stored stats for the given collection, does not error
+     * for collection not found as that's a legitimate state (and returns 0 for
+     * all stats).
+     * @param vbid of the vbucket to get collections stats from
+     * @param collection the id of the collection to lookup
+     * @return pair of GetCollectionStatsStatus and the stats. The status can
+     *         be Success for when stats for the collection were found.
+     *         NotFound for when no stats were found and the returned stats are
+     *         default initialised. Finally Failed can occur for unexpected
+     *         errors.
+     */
+    virtual std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>
+    getCollectionStats(Vbid vbid, CollectionID collection) const = 0;
+
     /**
      * Note that for disk snapshots that have no manifest yet written, a uid
      * of zero is returned (the default state).

engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc

@@ -2111,7 +2111,6 @@ bool MagmaKVStore::compactDBInternal(std::unique_lock<std::mutex>& vbLock,
                               Collections::VB::PersistedStats>>
                 dcInfo;
         for (auto& dc : dropped) {
-            auto handle = makeFileHandle(vbid);
             auto [status, stats] =
                     getDroppedCollectionStats(vbid, dc.collectionId);
             if (status == KVStore::GetCollectionStatsStatus::Failed) {
@@ -2635,20 +2634,21 @@ void MagmaKVStore::saveCollectionStats(
     localDbReqs.emplace_back(MagmaLocalReq(key, std::move(encodedStats)));
 }
 
-std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
-MagmaKVStore::getDroppedCollectionStats(Vbid vbid, CollectionID cid) {
-    auto key = getDroppedCollectionsStatsKey(cid);
-    return getCollectionStats(vbid, key);
-}
-
 std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
 MagmaKVStore::getCollectionStats(const KVFileHandle& kvFileHandle,
                                  CollectionID cid) const {
+    // TODO: update this to use snapshot
     const auto& kvfh = static_cast<const MagmaKVFileHandle&>(kvFileHandle);
     auto vbid = kvfh.vbid;
     return getCollectionStats(vbid, cid);
 }
 
+std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
+MagmaKVStore::getDroppedCollectionStats(Vbid vbid, CollectionID cid) {
+    auto key = getDroppedCollectionsStatsKey(cid);
+    return getCollectionStats(vbid, key);
+}
+
 std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
 MagmaKVStore::getCollectionStats(Vbid vbid, CollectionID cid) const {
     auto key = getCollectionsStatsKey(cid);

engines/ep/src/kvstore/magma-kvstore/magma-kvstore.h

@@ -244,15 +244,8 @@ class MagmaKVStore : public KVStore {
     getCollectionStats(const KVFileHandle& kvFileHandle,
                        CollectionID collection) const override;
 
-    /**
-     * Get the alive collection stats for the given collection
-     *
-     * @param vbid
-     * @param collection to find stats for
-     * @return Bool status and Stats (defaulted to 0 if not found)
-     */
     std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>
-    getCollectionStats(Vbid, CollectionID collection) const;
+    getCollectionStats(Vbid, CollectionID collection) const override;
 
     /**
      * Get the collection stats for the given key

engines/ep/src/kvstore/nexus-kvstore/nexus-kvstore.cc

@@ -79,16 +79,13 @@ Collections::VB::Manifest NexusKVStore::generateSecondaryVBManifest(
     // Having generated the Manifest object we now need to correct the disk
     // sizes as they may differ between KVStores. We'll load the disk sizes of
     // each collection now...
-    auto secondaryFileHandle = secondary->makeFileHandle(vbid);
-    if (secondaryFileHandle) {
-        auto collections = secondaryManifest.wlock();
-        for (auto& itr : collections) {
-            auto& [cid, entry] = itr;
-            auto [status, stats] =
-                    secondary->getCollectionStats(*secondaryFileHandle, cid);
-            if (status == GetCollectionStatsStatus::Success) {
-                collections.setDiskSize(cid, stats.diskSize);
-            }
+
+    auto collections = secondaryManifest.wlock();
+    for (auto& itr : collections) {
+        auto& [cid, entry] = itr;
+        auto [status, stats] = secondary->getCollectionStats(vbid, cid);
+        if (status == GetCollectionStatsStatus::Success) {
+            collections.setDiskSize(cid, stats.diskSize);
         }
     }
 
@@ -128,28 +125,11 @@ void NexusKVStore::doCollectionsMetadataChecks(
     for (const auto& collection : primaryKVStoreManifest.collections) {
         auto& cid = collection.metaData.cid;
 
-        auto primaryHandle = primary->makeFileHandle(vbid);
-        if (!primaryHandle) {
-            auto msg = fmt::format(
-                    "NexusKVStore::doCollectionsMetadataChecks: {}: issue "
-                    "getting primary file handle",
-                    vbid);
-            handleError(msg);
-        }
-
         auto [primaryResult, primaryStats] =
-                primary->getCollectionStats(*primaryHandle, cid);
-        auto secondaryHandle = secondary->makeFileHandle(vbid);
-        if (!secondaryHandle) {
-            auto msg = fmt::format(
-                    "NexusKVStore::doCollectionsMetadataChecks: {}: issue "
-                    "getting secondary file handle",
-                    vbid);
-            handleError(msg);
-        }
+                primary->getCollectionStats(vbid, cid);
 
         auto [secondaryResult, secondaryStats] =
-                secondary->getCollectionStats(*secondaryHandle, cid);
+                secondary->getCollectionStats(vbid, cid);
         if (primaryResult != secondaryResult) {
             auto msg = fmt::format(
                     "NexusKVStore::doCollectionsMetadataChecks: {}: issue "
@@ -1058,6 +1038,11 @@ NexusKVStore::getCollectionStats(const KVFileHandle& kvFileHandle,
     return primary->getCollectionStats(kvFileHandle, collection);
 }
 
+std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
+NexusKVStore::getCollectionStats(Vbid vbid, CollectionID collection) const {
+    return primary->getCollectionStats(vbid, collection);
+}
+
 std::optional<Collections::ManifestUid> NexusKVStore::getCollectionsManifestUid(
         KVFileHandle& kvFileHandle) {
     return primary->getCollectionsManifestUid(kvFileHandle);

engines/ep/src/kvstore/nexus-kvstore/nexus-kvstore.h

@@ -100,6 +100,8 @@ class NexusKVStore : public KVStoreIface {
     std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>
     getCollectionStats(const KVFileHandle& kvFileHandle,
                        CollectionID collection) const override;
+    std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>
+    getCollectionStats(Vbid vbid, CollectionID collection) const override;
     std::optional<Collections::ManifestUid> getCollectionsManifestUid(
             KVFileHandle& kvFileHandle) override;
     std::pair<bool, Collections::KVStore::Manifest> getCollectionsManifest(

engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.cc

@@ -1419,6 +1419,14 @@ RocksDBKVStore::getCollectionStats(const KVFileHandle& kvFileHandle,
             Collections::VB::PersistedStats()};
 }
 
+std::pair<KVStore::GetCollectionStatsStatus, Collections::VB::PersistedStats>
+RocksDBKVStore::getCollectionStats(Vbid vbid, CollectionID collection) const {
+    // TODO JWW 2018-07-30 implement this, for testing purposes return dummy
+    // values to imply the function didn't fail
+    return {GetCollectionStatsStatus::Success,
+            Collections::VB::PersistedStats()};
+}
+
 std::unique_ptr<BySeqnoScanContext> RocksDBKVStore::initBySeqnoScanContext(
         std::unique_ptr<StatusCallback<GetValue>> cb,
         std::unique_ptr<StatusCallback<CacheLookup>> cl,

engines/ep/src/kvstore/rocksdb-kvstore/rocksdb-kvstore.h

@@ -286,6 +286,9 @@ class RocksDBKVStore : public KVStore {
     getCollectionStats(const KVFileHandle& kvFileHandle,
                        CollectionID collection) const override;
 
+    std::pair<GetCollectionStatsStatus, Collections::VB::PersistedStats>
+    getCollectionStats(Vbid vbid, CollectionID collection) const override;
+
     void prepareToCreateImpl(Vbid vbid) override {
         // TODO DJR 2017-05-19 implement this.
     }

engines/ep/tests/mock/mock_kvstore.h

@@ -175,6 +175,11 @@ class MockKVStore : public KVStore {
                 getCollectionStats,
                 (const KVFileHandle& kvFileHandle, CollectionID collection),
                 (const, override));
+    MOCK_METHOD((std::pair<KVStore::GetCollectionStatsStatus,
+                           Collections::VB::PersistedStats>),
+                getCollectionStats,
+                (Vbid vbid, CollectionID collection),
+                (const, override));
     MOCK_METHOD((std::optional<Collections::ManifestUid>),
                 getCollectionsManifestUid,
                 (KVFileHandle & kvFileHandle),

engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc

@@ -3193,13 +3193,13 @@ void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd,
         EXPECT_TRUE(fileHandle);
 
         if (dropAtEnd) {
-            auto [status, stats] = kvs.getCollectionStats(*fileHandle, target);
+            auto [status, stats] = kvs.getCollectionStats(id, target);
             EXPECT_EQ(KVStore::GetCollectionStatsStatus::NotFound, status);
             EXPECT_EQ(0, stats.itemCount);
             EXPECT_EQ(0, stats.highSeqno);
             EXPECT_EQ(0, stats.diskSize);
         } else {
-            auto [status, stats] = kvs.getCollectionStats(*fileHandle, target);
+            auto [status, stats] = kvs.getCollectionStats(id, target);
             EXPECT_EQ(KVStore::GetCollectionStatsStatus::Success, status);
             EXPECT_EQ(expected, stats.itemCount);
             EXPECT_EQ(7, stats.highSeqno);
@@ -3368,9 +3368,8 @@ void CollectionsDcpPersistentOnly::resurrectionStatsTest(
     EXPECT_EQ(highSeqno, stats.highSeqno);
 
     auto kvstore = store->getRWUnderlying(vbid);
-    auto kvstoreHandle = kvstore->makeFileHandle(vbid);
     auto [status, diskStats] =
-            kvstore->getCollectionStats(*kvstoreHandle, target.getId());
+            kvstore->getCollectionStats(vbid, target.getId());
     ASSERT_EQ(status, KVStore::GetCollectionStatsStatus::Success);
     EXPECT_EQ(highSeqno, diskStats.highSeqno);
     EXPECT_EQ(systemeventSize + itemSize, diskStats.diskSize);
@@ -3389,9 +3388,8 @@ void CollectionsDcpPersistentOnly::resurrectionStatsTest(
     highSeqno++;
     EXPECT_EQ(highSeqno, stats.highSeqno);
 
-    kvstoreHandle = kvstore->makeFileHandle(vbid);
     std::tie(status, diskStats) =
-            kvstore->getCollectionStats(*kvstoreHandle, target.getId());
+            kvstore->getCollectionStats(vbid, target.getId());
     EXPECT_EQ(highSeqno, diskStats.highSeqno);
     EXPECT_EQ(systemeventSize + itemSize, diskStats.diskSize);
     EXPECT_EQ(0, diskStats.itemCount);

engines/ep/tests/module_tests/collections/evp_store_collections_eraser_test.cc

@@ -1608,15 +1608,12 @@ void CollectionsEraserPersistentOnly::testEmptyCollections(
         EXPECT_EQ(2, handle.getHighSeqno(CollectionEntry::fruit));
         EXPECT_EQ(2, handle.getPersistedHighSeqno(CollectionEntry::fruit));
 
-        auto fileHandle = kvs.makeFileHandle(vbid);
-        ASSERT_TRUE(fileHandle);
-        auto stats =
-                kvs.getCollectionStats(*fileHandle, CollectionEntry::dairy);
+        auto stats = kvs.getCollectionStats(vbid, CollectionEntry::dairy);
         EXPECT_EQ(KVStore::GetCollectionStatsStatus::Success, stats.first);
         EXPECT_EQ(0, stats.second.itemCount);
         EXPECT_EQ(vb->getHighSeqno() - 1, stats.second.highSeqno);
         EXPECT_NE(0, stats.second.diskSize);
-        stats = kvs.getCollectionStats(*fileHandle, CollectionEntry::fruit);
+        stats = kvs.getCollectionStats(vbid, CollectionEntry::fruit);
         EXPECT_EQ(KVStore::GetCollectionStatsStatus::Success, stats.first);
         EXPECT_EQ(0, stats.second.itemCount);
         EXPECT_EQ(vb->getHighSeqno(), stats.second.highSeqno);
@@ -1659,15 +1656,12 @@ void CollectionsEraserPersistentOnly::testEmptyCollections(
 
     if (!isMagma()) {
         // magma keeps the disk stats until next compaction completes
-        auto fileHandle = kvs.makeFileHandle(vbid);
-        ASSERT_TRUE(fileHandle);
-        auto stats =
-                kvs.getCollectionStats(*fileHandle, CollectionEntry::fruit);
+        auto stats = kvs.getCollectionStats(vbid, CollectionEntry::fruit);
         EXPECT_EQ(KVStore::GetCollectionStatsStatus::NotFound, stats.first);
         EXPECT_EQ(0, stats.second.itemCount);
         EXPECT_EQ(0, stats.second.highSeqno);
         EXPECT_EQ(0, stats.second.diskSize);
-        stats = kvs.getCollectionStats(*fileHandle, CollectionEntry::dairy);
+        stats = kvs.getCollectionStats(vbid, CollectionEntry::dairy);
         EXPECT_EQ(KVStore::GetCollectionStatsStatus::NotFound, stats.first);
         EXPECT_EQ(0, stats.second.itemCount);
         EXPECT_EQ(0, stats.second.highSeqno);

engines/ep/tests/module_tests/collections/evp_store_collections_test.cc

@@ -1894,12 +1894,10 @@ TEST_P(ConcurrentCompactPurge, ConcCompactPurgeTombstones) {
 
     auto compareDiskStatMemoryVsPersisted = [vb]() {
         auto kvs = vb->getShard()->getRWUnderlying();
-        auto fileHandle = kvs->makeFileHandle(vb->getId());
-        ASSERT_TRUE(fileHandle);
         auto fruitSz =
                 vb->getManifest().lock(CollectionEntry::fruit).getDiskSize();
         auto stats =
-                kvs->getCollectionStats(*fileHandle, CollectionEntry::fruit);
+                kvs->getCollectionStats(vb->getId(), CollectionEntry::fruit);
         EXPECT_EQ(KVStore::GetCollectionStatsStatus::Success, stats.first);
         EXPECT_EQ(stats.second.diskSize, fruitSz);
     };
@@ -3668,17 +3666,15 @@ TEST_P(CollectionsCouchstoreParameterizedTest, TombstonePurge) {
 
     auto compareDiskStatMemoryVsPersisted = [vb]() {
         auto kvs = vb->getShard()->getRWUnderlying();
-        auto fileHandle = kvs->makeFileHandle(vb->getId());
-        ASSERT_TRUE(fileHandle);
         auto fruitSz =
                 vb->getManifest().lock(CollectionEntry::fruit).getDiskSize();
         auto dairySz =
                 vb->getManifest().lock(CollectionEntry::dairy).getDiskSize();
         auto stats =
-                kvs->getCollectionStats(*fileHandle, CollectionEntry::fruit);
+                kvs->getCollectionStats(vb->getId(), CollectionEntry::fruit);
         EXPECT_EQ(KVStore::GetCollectionStatsStatus::Success, stats.first);
         EXPECT_EQ(stats.second.diskSize, fruitSz);
-        stats = kvs->getCollectionStats(*fileHandle, CollectionEntry::dairy);
+        stats = kvs->getCollectionStats(vb->getId(), CollectionEntry::dairy);
         EXPECT_EQ(KVStore::GetCollectionStatsStatus::Success, stats.first);
 
         EXPECT_EQ(stats.second.diskSize, dairySz);

engines/ep/tests/module_tests/collections/stat_checker.cc

@@ -52,9 +52,7 @@ StatChecker::~StatChecker() {
         return;
     }
     auto kvs = vb->getShard()->getRWUnderlying();
-    auto fileHandle = kvs->makeFileHandle(vb->getId());
-    EXPECT_TRUE(fileHandle);
-    auto stats = kvs->getCollectionStats(*fileHandle, entry.uid);
+    auto stats = kvs->getCollectionStats(vb->getId(), entry.uid);
     EXPECT_EQ(KVStore::GetCollectionStatsStatus::Success, stats.first);
 
     // Regardless of the postCondition the following should all be equal