MB-49040: 6/n Only check limits when limits are present

jimwwalker · jimwwalker · commit 4b3c037483fe · 2021-11-01T16:40:53.000Z
Avoid a scope map lookup + value compare if there are no limits to check. A VB:Manifest flag now exists that is set everytime a VB::Manifest is updated against a Manifest from the cluster. If any scope is found to have a limit, we enable the flag or disable if no scopes with limits are found. Change-Id: I83a7062822d6eb7805164e411600a6287b3bf737 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/164342 Tested-by: Build Bot <build@couchbase.com> Reviewed-by: Dave Rigby <daver@couchbase.com>
diff --git a/engines/ep/src/collections/manifest.cc b/engines/ep/src/collections/manifest.cc
@@ -769,7 +769,7 @@ std::ostream& operator<<(std::ostream& os, const Manifest& manifest) {
        << ", uid:" << manifest.uid
        << ", collections.size:" << manifest.collections.size() << std::endl;
     for (const auto& entry : manifest.scopes) {
-        os << "scope:{" << entry.first.to_string() << ", " << entry.second.name;
+        os << "scope:{" << entry.first << ", " << entry.second.name;
 
         if (entry.second.dataLimit) {
             os << ", dataLimit:" << entry.second.dataLimit.value()
@@ -779,8 +779,8 @@ std::ostream& operator<<(std::ostream& os, const Manifest& manifest) {
         os << ", collections:[";
 
         for (const auto& collection : entry.second.collections) {
-            os << "{cid:" << collection.cid.to_string() << ", sid:" << std::hex
-               << collection.sid << ", " << collection.name
+            os << "{cid:" << collection.cid << ", sid:" << collection.sid
+               << ", " << collection.name
                << ", ttl:" << (collection.maxTtl.has_value() ? "yes" : "no")
                << ":"
                << collection.maxTtl.value_or(std::chrono::seconds(0)).count()
@@ -790,9 +790,8 @@ std::ostream& operator<<(std::ostream& os, const Manifest& manifest) {
     }
 
     for (const auto& [key, collection] : manifest.collections) {
-        os << "{cid key:" << key.to_string()
-           << ", value:" << collection.cid.to_string() << ", sid:" << std::hex
-           << collection.sid << ", " << collection.name
+        os << "{cid key:" << key.to_string() << ", value:" << collection.cid
+           << ", sid:" << collection.sid << ", " << collection.name
            << ", ttl:" << (collection.maxTtl.has_value() ? "yes" : "no") << ":"
            << collection.maxTtl.value_or(std::chrono::seconds(0)).count()
            << "}";
diff --git a/engines/ep/src/collections/vbucket_manifest.cc b/engines/ep/src/collections/vbucket_manifest.cc
@@ -237,27 +237,26 @@ ManifestUpdateStatus Manifest::update(VBucket& vb,
 
     // If manifest was equal.
     if (manifest.getUid() == manifestUid) {
-        if (changes.empty()) {
+        if (changes.none()) {
             return ManifestUpdateStatus::Success;
-        } else if (changes.onlyScopesModified()) {
-            EP_LOG_WARN("Manifest::update {} equal uid and {} scopes modified",
-                        vb.getId(),
-                        changes.scopesToModify.size());
-        } else {
-            // Log verbosely for this case
+        } else if (changes.wouldCreateOrDrop()) {
+            // Log verbosely for this case. An equal manifest should not change
+            // the scope or collection membership
             EP_LOG_WARN(
                     "Manifest::update {} with equal uid:{:#x} but differences "
-                    "scopes+:{}, collections+:{}, scopes-:{}, collections-:{}, "
-                    "scopes-modified:{}",
+                    "scopes+:{}, collections+:{}, scopes-:{}, collections-:{}",
                     vb.getId(),
                     manifestUid,
                     changes.scopesToCreate.size(),
                     changes.collectionsToCreate.size(),
                     changes.scopesToDrop.size(),
-                    changes.collectionsToDrop.size(),
-                    changes.scopesToModify.size());
+                    changes.collectionsToDrop.size());
             return ManifestUpdateStatus::EqualUidWithDifferences;
         }
+        // else a scope modification or change of scopeWithDataLimitExists is
+        // allowed when the uid is equal
+        Expects(!changes.scopesToModify.empty() ||
+                changes.changeScopeWithDataLimitExists);
     }
 
     completeUpdate(std::move(upgradeLock), vb, changes);
@@ -292,25 +291,39 @@ void Manifest::completeUpdate(mutex_type::UpgradeHolder&& upgradeLock,
     WriteHandle wHandle(*this, std::move(upgradeLock));
 
     // Capture the UID
-    if (changeset.empty()) {
+    if (changeset.none()) {
         updateUid(changeset.uid, false /*reset*/);
         return;
     }
 
+    if (changeset.changeScopeWithDataLimitExists) {
+        EP_LOG_INFO(
+                "Manifest::completeUpdate {} toggling scopeWithDataLimitExists "
+                "{} -> {}",
+                vb.getId(),
+                scopeWithDataLimitExists,
+                changeset.changeScopeWithDataLimitExists.value());
+        scopeWithDataLimitExists =
+                changeset.changeScopeWithDataLimitExists.value();
+    }
+
     auto finalDeletion = applyDrops(wHandle, vb, changeset.collectionsToDrop);
     if (finalDeletion) {
-        auto uid = changeset.empty() ? changeset.uid : manifestUid;
-        dropCollection(
-                wHandle, vb, uid, *finalDeletion, OptionalSeqno{/*no-seqno*/});
+        // if the changeset is now 'drained' of create/drop, then this is final
+        // the final change to make - so use changeset.uid
+        dropCollection(wHandle,
+                       vb,
+                       changeset.getUidForChange(manifestUid),
+                       *finalDeletion,
+                       OptionalSeqno{/*no-seqno*/});
     }
 
     auto finalScopeCreate =
             applyScopeCreates(wHandle, vb, changeset.scopesToCreate);
     if (finalScopeCreate) {
-        auto uid = changeset.empty() ? changeset.uid : manifestUid;
         createScope(wHandle,
                     vb,
-                    uid,
+                    changeset.getUidForChange(manifestUid),
                     finalScopeCreate.value().sid,
                     finalScopeCreate.value().name,
                     finalScopeCreate.value().dataLimit,
@@ -321,10 +334,9 @@ void Manifest::completeUpdate(mutex_type::UpgradeHolder&& upgradeLock,
             applyCreates(wHandle, vb, changeset.collectionsToCreate);
 
     if (finalAddition) {
-        auto uid = changeset.empty() ? changeset.uid : manifestUid;
         createCollection(wHandle,
                          vb,
-                         uid,
+                         changeset.getUidForChange(manifestUid),
                          finalAddition.value().identifiers,
                          finalAddition.value().name,
                          finalAddition.value().maxTtl,
@@ -346,6 +358,7 @@ void Manifest::completeUpdate(mutex_type::UpgradeHolder&& upgradeLock,
     // Can do the scope modifications last - these generate no events but will
     // just sync any scope to the manifest
     for (const auto& modified : changeset.scopesToModify) {
+        // Here the changeset.uid is used as it's for logging
         modifyScope(wHandle, vb, changeset.uid, modified);
     }
 }
@@ -696,6 +709,8 @@ Manifest::ManifestChanges Manifest::processManifest(
         const Collections::Manifest& manifest) {
     ManifestChanges rv{manifest.getUid()};
 
+    bool scopeWithDataLimitExists{false};
+
     // First iterate through the collections of this VB::Manifest
     for (const auto& [cid, entry] : map) {
         // Look-up the collection inside the new manifest
@@ -737,6 +752,10 @@ Manifest::ManifestChanges Manifest::processManifest(
             }
         }
 
+        if (scopeItr->second.dataLimit) {
+            scopeWithDataLimitExists = true;
+        }
+
         for (const auto& m : scopeItr->second.collections) {
             auto mapItr = map.find(m.cid);
 
@@ -747,6 +766,10 @@ Manifest::ManifestChanges Manifest::processManifest(
         }
     }
 
+    if (this->scopeWithDataLimitExists != scopeWithDataLimitExists) {
+        rv.changeScopeWithDataLimitExists = scopeWithDataLimitExists;
+    }
+
     return rv;
 }
 
@@ -1167,6 +1190,10 @@ bool Manifest::addScopeStats(Vbid vbid, const StatCollector& collector) const {
     format_to(key, "vb_{}:manifest:uid", vbid.get());
     collector.addStat(std::string_view(key.data(), key.size()), manifestUid);
 
+    format_to(key, "vb_{}:scope_data_limit", vbid.get());
+    collector.addStat(std::string_view(key.data(), key.size()),
+                      scopeWithDataLimitExists);
+
     for (const auto& [sid, value] : scopes) {
         key.resize(0);
         format_to(key, "vb_{}:{}:name:", vbid.get(), sid);
@@ -1273,6 +1300,10 @@ bool Manifest::operator!=(const Manifest& rhs) const {
 
 cb::engine_errc Manifest::getScopeDataLimitStatus(
         const container::const_iterator itr, size_t nBytes) const {
+    if (!scopeWithDataLimitExists) {
+        return cb::engine_errc::success;
+    }
+
     const auto& entry = getScopeEntry(itr->second.getScopeID());
 
     if (entry.getDataLimit() &&
@@ -1446,6 +1477,8 @@ std::ostream& operator<<(std::ostream& os,
 std::ostream& operator<<(std::ostream& os, const Manifest& manifest) {
     os << "VB::Manifest: "
        << "uid:" << manifest.manifestUid
+       << "scopeWithDataLimitExists:" << manifest.scopeWithDataLimitExists
+       << "dropInProgress:" << manifest.dropInProgress.load()
        << ", scopes.size:" << manifest.scopes.size()
        << ", map.size:" << manifest.map.size() << std::endl;
     for (const auto& [cid, entry] : manifest.map) {
diff --git a/engines/ep/src/collections/vbucket_manifest.h b/engines/ep/src/collections/vbucket_manifest.h
@@ -318,18 +318,29 @@ class Manifest {
         std::vector<ScopeModified> scopesToModify;
         std::vector<CollectionCreation> collectionsToCreate;
         std::vector<CollectionID> collectionsToDrop;
+        // stores any new value (if needed)
+        std::optional<bool> changeScopeWithDataLimitExists;
+
         const ManifestUid uid{0};
 
-        bool empty() const {
+        /// @return true if no changes are to be made
+        bool none() const {
             return scopesToCreate.empty() && scopesToModify.empty() &&
                    scopesToDrop.empty() && collectionsToCreate.empty() &&
-                   collectionsToDrop.empty();
+                   collectionsToDrop.empty() && !changeScopeWithDataLimitExists;
+        }
+
+        /// @return true if there are collections/scopes to create or drop
+        bool wouldCreateOrDrop() const {
+            return !(scopesToCreate.empty() && scopesToDrop.empty() &&
+                     collectionsToCreate.empty() && collectionsToDrop.empty());
         }
 
-        bool onlyScopesModified() const {
-            return scopesToCreate.empty() && scopesToDrop.empty() &&
-                   collectionsToCreate.empty() && collectionsToDrop.empty() &&
-                   !scopesToModify.empty();
+        ManifestUid getUidForChange(ManifestUid theirUid) {
+            // If the set of changes no longer will create or drop, i.e. is now
+            // 'drained' of creates or drops then the correct uid to now use is
+            // the change set uid
+            return !wouldCreateOrDrop() ? uid : theirUid;
         }
     };
 
@@ -1035,6 +1046,12 @@ class Manifest {
     /// Manager of collections
     const std::shared_ptr<Manager> manager;
 
+    /**
+     * Flag to help avoid limit checking when there are no limits to check.
+     * This is written with the write lock and only readable via the read lock.
+     */
+    bool scopeWithDataLimitExists{false};
+
     /**
      * Flag value which indicates if dropped collections exist, that includes
      * documents and metadata. This is atomic because we don't require the
diff --git a/engines/ep/tests/module_tests/collections/scope_data_limit_tests.cc b/engines/ep/tests/module_tests/collections/scope_data_limit_tests.cc
@@ -46,7 +46,7 @@ TEST_P(CollectionsDcpParameterizedTest,
     store->setVBucketState(vbid, vbucket_state_replica);
 
     // No active vbuckets, so shop1 isn't applied anywhere yet
-    const size_t limit = 1024;
+    const size_t limit = 10;
     CollectionsManifest cm;
     cm.add(ScopeEntry::shop1,
            limit * store->getEPEngine().getConfiguration().getMaxVbuckets());
@@ -83,6 +83,36 @@ TEST_P(CollectionsDcpParameterizedTest,
     EXPECT_TRUE(vb->getManifest().lock().getDataLimit(ScopeEntry::shop1));
     EXPECT_EQ(limit,
               vb->getManifest().lock().getDataLimit(ScopeEntry::shop1).value());
+
+    // Check we fail limits on the active vbucket
+    store_item(vbid,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit + 1, 'a'),
+               0,
+               {cb::engine_errc::scope_size_limit_exceeded});
+    store_item(vbid,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit, 'a'));
+
+    // for completeness - the other vb will reject for  other reasons
+    store_item(replicaVB,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit + 1, 'a'),
+               0,
+               {cb::engine_errc::not_my_vbucket});
+
+    // And check when active it rejects
+    store->setVBucketState(replicaVB, vbucket_state_active);
+
+    store_item(replicaVB,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit + 1, 'a'),
+               0,
+               {cb::engine_errc::scope_size_limit_exceeded});
+    // but ok for at the limit
+    store_item(replicaVB,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit, 'a'));
 }
 
 // Test that when an active vbucket has already applied the scope with limit
@@ -101,7 +131,7 @@ TEST_P(CollectionsDcpParameterizedTest,
 TEST_P(CollectionsDcpParameterizedTest,
        replica_gets_scope_with_limit_from_active) {
     CollectionsManifest cm;
-    const size_t limit = 1024;
+    const size_t limit = 10;
     cm.add(ScopeEntry::shop1,
            limit * store->getEPEngine().getConfiguration().getMaxVbuckets());
     cm.add(CollectionEntry::fruit, ScopeEntry::shop1);
@@ -146,6 +176,29 @@ TEST_P(CollectionsDcpParameterizedTest,
     EXPECT_TRUE(vb->getManifest().lock().getDataLimit(ScopeEntry::shop1));
     EXPECT_EQ(limit,
               vb->getManifest().lock().getDataLimit(ScopeEntry::shop1).value());
+
+    // Check we fail limits on both vbuckets if the size is right.
+    // If we have the scopeWithDataLimitExists code wrong, the limits wouldn't
+    // be checked
+    store_item(vbid,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit + 1, 'a'),
+               0,
+               {cb::engine_errc::scope_size_limit_exceeded});
+
+    store_item(replicaVB,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit + 1, 'a'),
+               0,
+               {cb::engine_errc::scope_size_limit_exceeded});
+
+    // But can write upto the limit
+    store_item(replicaVB,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit, 'a'));
+    store_item(vbid,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit, 'a'));
 }
 
 // Test that when only replicas exist and they are the first to create a scope
@@ -203,7 +256,7 @@ TEST_P(CollectionsDcpParameterizedTest, active_updates_limit) {
 
     // Now set shop1 with limit
     CollectionsManifest cm;
-    const size_t limit = 1024;
+    const size_t limit = 10;
     cm.add(ScopeEntry::shop1,
            limit * store->getEPEngine().getConfiguration().getMaxVbuckets());
     cm.add(CollectionEntry::fruit, ScopeEntry::shop1);
@@ -220,6 +273,29 @@ TEST_P(CollectionsDcpParameterizedTest, active_updates_limit) {
                       .lock()
                       .getDataLimit(ScopeEntry::shop1)
                       .value());
+
+    // Check we fail limits on both vbuckets if the size is right.
+    // If we have the scopeWithDataLimitExists code wrong, the limits wouldn't
+    // be checked
+    store_item(vbid,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit + 1, 'a'),
+               0,
+               {cb::engine_errc::scope_size_limit_exceeded});
+
+    store_item(replicaVB,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit + 1, 'a'),
+               0,
+               {cb::engine_errc::scope_size_limit_exceeded});
+
+    // But can write upto the limit
+    store_item(replicaVB,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit, 'a'));
+    store_item(vbid,
+               StoredDocKey{"k1", CollectionEntry::fruit},
+               std::string(limit, 'a'));
 }
 
 TEST_F(CollectionsTest, ScopeWithManyCollectionsWarmup) {
@@ -253,4 +329,4 @@ TEST_F(CollectionsTest, ScopeWithManyCollectionsWarmup) {
     EXPECT_EQ(ds,
               store->getVBucket(vbid)->getManifest().lock().getDataSize(
                       ScopeEntry::shop1));
-}
+}
diff --git a/engines/ep/tests/module_tests/collections/vbucket_manifest_test.cc b/engines/ep/tests/module_tests/collections/vbucket_manifest_test.cc
