MB-27554: [BP] Don't over-count VBucket item counts

daverigby · daverigby · commit 46152ca14c36 · 2018-01-29T09:53:59.000Z
Originally 526b3f4 TODO: Check what happens in FE if an item is created and deleted in the same flusher step - I believe the create will be de-duplicated with the delete; so only the delete persistence callback will run. In that case I think the item count may be incorrect - or at the very least it may require newCacheItem to handle it correctly. Change-Id: Id131081e9e80436fcc861ce9f917be040d74b430 Reviewed-on: http://review.couchbase.org/88380 Well-Formed: Build Bot <build@couchbase.com> Tested-by: Build Bot <build@couchbase.com> Reviewed-by: Dave Rigby <daver@couchbase.com>
diff --git a/engines/ep/src/ep_bucket.cc b/engines/ep/src/ep_bucket.cc
@@ -596,12 +596,16 @@ ENGINE_ERROR_CODE EPBucket::scheduleCompaction(uint16_t vbid,
 void EPBucket::flushOneDeleteAll() {
     for (VBucketMap::id_type i = 0; i < vbMap.getSize(); ++i) {
         VBucketPtr vb = getVBucket(i);
+        if (!vb) {
+            continue;
+        }
         // Reset the vBucket if it's non-null and not already in the middle of
         // being created / destroyed.
-        if (vb && !(vb->isBucketCreation() || vb->isDeletionDeferred())) {
-            LockHolder lh(vb_mutexes[vb->getId()]);
+        if (!(vb->isBucketCreation() || vb->isDeletionDeferred())) {
             getRWUnderlying(vb->getId())->reset(i);
         }
+        // Reset disk item count.
+        vb->ht.setNumTotalItems(0);
     }
 
     --stats.diskQueueSize;
@@ -909,12 +913,7 @@ class EPDiskRollbackCB : public RollbackCB {
         if (gcb.getStatus() == ENGINE_SUCCESS) {
             UniqueItemPtr it(std::move(gcb.item));
             if (it->isDeleted()) {
-                bool ret = vb->deleteKey(it->getKey());
-                if (!ret) {
-                    setStatus(ENGINE_KEY_ENOENT);
-                } else {
-                    setStatus(ENGINE_SUCCESS);
-                }
+                removeDeletedDoc(*vb, it->getKey());
             } else {
                 MutationStatus mtype = vb->setFromInternal(*it);
 
@@ -923,19 +922,28 @@ class EPDiskRollbackCB : public RollbackCB {
                 }
             }
         } else if (gcb.getStatus() == ENGINE_KEY_ENOENT) {
-            bool ret = vb->deleteKey(itm->getKey());
-            if (!ret) {
-                setStatus(ENGINE_KEY_ENOENT);
-            } else {
-                setStatus(ENGINE_SUCCESS);
-            }
+            removeDeletedDoc(*vb, itm->getKey());
         } else {
             LOG(EXTENSION_LOG_WARNING,
                 "EPDiskRollbackCB::callback:Unexpected Error Status: %d",
                 gcb.getStatus());
         }
     }
 
+    /// Remove a deleted-on-disk document from the VBucket's hashtable.
+    void removeDeletedDoc(VBucket& vb, const DocKey& key) {
+        if (vb.deleteKey(key)) {
+            setStatus(ENGINE_SUCCESS);
+        } else {
+            // Document didn't exist in memory - may have been deleted in since
+            // the checkpoint.
+            setStatus(ENGINE_KEY_ENOENT);
+        }
+        // Irrespective of if the in-memory delete succeeded; the document
+        // doesn't exist on disk; so decrement the item count.
+        vb.ht.decrNumTotalItems();
+    }
+
 private:
     EventuallyPersistentEngine& engine;
 };
diff --git a/engines/ep/src/ep_vb.cc b/engines/ep/src/ep_vb.cc
@@ -257,15 +257,15 @@ size_t EPVBucket::getNumItems() const {
     if (eviction == VALUE_ONLY) {
         return ht.getNumInMemoryItems() - ht.getNumDeletedItems();
     } else {
-        return ht.getNumItems() - ht.getNumDeletedItems();
+        return ht.getNumTotalItems() - ht.getNumDeletedItems();
     }
 }
 
 size_t EPVBucket::getNumNonResidentItems() const {
     if (eviction == VALUE_ONLY) {
         return ht.getNumInMemoryNonResItems();
     } else {
-        size_t num_items = ht.getNumItems();
+        size_t num_items = ht.getNumTotalItems();
         size_t num_res_items =
                 ht.getNumInMemoryItems() - ht.getNumInMemoryNonResItems();
         return num_items > num_res_items ? (num_items - num_res_items) : 0;
diff --git a/engines/ep/src/hash_table.cc b/engines/ep/src/hash_table.cc
@@ -324,7 +324,6 @@ void HashTable::statsPrologue(const StoredValue& v) {
         --numTempItems;
     } else {
         --numItems;
-        --numTotalItems;
         if (v.isDeleted()) {
             --numDeletedItems;
         } else {
@@ -346,7 +345,6 @@ void HashTable::statsEpilogue(const StoredValue& v) {
         ++numTempItems;
     } else {
         ++numItems;
-        ++numTotalItems;
         if (v.isDeleted()) {
             ++numDeletedItems;
         } else {
@@ -475,9 +473,6 @@ MutationStatus HashTable::insertFromWarmup(
             v->markNotResident();
             ++numNonResidentItems;
         }
-        /* We need to decrNumTotalItems because ht.numTotalItems is already
-         set by warmup when it estimated the item count from disk */
-        decrNumTotalItems();
         v->setNewCacheItem(false);
     } else {
         if (keyMetaDataOnly) {
@@ -746,12 +741,6 @@ bool HashTable::unlocked_restoreValue(
 
     if (v.isDeleted()) {
         ++numDeletedItems;
-        // We have restored a deleted item. This doesn't count as an alive
-        // item, however it does contribute to the total number of documents
-        // in the HashTable, hence we need to also increment numTotalItems.
-        // Note: at the vBucket level we determine the number of (alive) items
-        // as (numTotal - numDeleted).
-        ++numTotalItems;
     }
 
     increaseCacheSize(v.getValue()->length());
diff --git a/engines/ep/src/hash_table.h b/engines/ep/src/hash_table.h
@@ -222,21 +222,27 @@ class HashTable {
     /**
      * Get the number of non-resident and resident items managed by
      * this hash table. Includes items marked as deleted.
-     * Note that this will be equal to numItems if
-     * VALUE_ONLY_EVICTION is chosen as a cache management.
      */
-    size_t getNumItems(void) const {
-        return numTotalItems;
+    size_t getNumItems() const {
+        return numItems;
     }
 
     void setNumTotalItems(size_t totalItems) {
         numTotalItems = totalItems;
     }
 
+    size_t getNumTotalItems() const {
+        return numTotalItems;
+    }
+
     void decrNumItems() {
         --numItems;
     }
 
+    void incrNumTotalItems() {
+        ++numTotalItems;
+    }
+
     void decrNumTotalItems() {
         --numTotalItems;
     }
diff --git a/engines/ep/src/persistence_callback.cc b/engines/ep/src/persistence_callback.cc
@@ -51,9 +51,9 @@ void PersistenceCallback::callback(mutation_result& value) {
                 if (value.second) {
                     // Insert in value-only or full eviction mode.
                     ++vbucket->opsCreate;
+                    vbucket->ht.incrNumTotalItems();
                     vbucket->incrMetaDataDisk(*queuedItem);
                 } else { // Update in full eviction mode.
-                    vbucket->ht.decrNumTotalItems();
                     ++vbucket->opsUpdate;
                 }
 
diff --git a/engines/ep/src/stored-value.cc b/engines/ep/src/stored-value.cc
@@ -290,6 +290,13 @@ bool StoredValue::deleteImpl() {
 }
 
 void StoredValue::setValueImpl(const Item& itm) {
+    if (isDeleted() && !itm.isDeleted()) {
+        // Transitioning from deleted -> alive - this should be considered
+        // a new cache item as it is increasing the number of (alive) items
+        // in the vBucket.
+        setNewCacheItem(true);
+    }
+
     setDeletedPriv(itm.isDeleted());
     flags = itm.getFlags();
     datatype = itm.getDataType();
diff --git a/engines/ep/src/vbucket.cc b/engines/ep/src/vbucket.cc
@@ -1870,7 +1870,6 @@ void VBucket::deletedOnDiskCbk(const Item& queuedItem, bool deleted) {
     //  1. Item is existent in hashtable, and deleted flag is true
     //  2. rev seqno of queued item matches rev seqno of hash table item
     if (v && v->isDeleted() && (queuedItem.getRevSeqno() == v->getRevSeqno())) {
-        bool newCacheItem = v->isNewCacheItem();
         bool isDeleted = deleteStoredValue(hbl, *v);
         if (!isDeleted) {
             throw std::logic_error(
@@ -1879,11 +1878,8 @@ void VBucket::deletedOnDiskCbk(const Item& queuedItem, bool deleted) {
                     std::to_string(v->getBySeqno()) + "' from bucket " +
                     std::to_string(hbl.getBucketNum()));
         }
-        if (newCacheItem && deleted) {
-            // Need to decrement the item counter again for an item that
-            // exists on DB file, but not in memory (i.e., full eviction),
-            // because we created the temp item in memory and incremented
-            // the item counter when a deletion is pushed in the queue.
+        if (deleted) {
+            // Removed an item from disk - decrement the count of total items.
             ht.decrNumTotalItems();
         }
 
diff --git a/engines/ep/tests/ep_testsuite.cc b/engines/ep/tests/ep_testsuite.cc
@@ -253,6 +253,7 @@ static int checkCurrItemsAfterShutdown(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1,
         keys.push_back(key);
     }
 
+    // Check preconditions.
     checkeq(0, get_int_stat(h, h1, "ep_total_persisted"),
             "Expected ep_total_persisted equals 0");
     checkeq(0, get_int_stat(h, h1, "curr_items"),
@@ -279,11 +280,15 @@ static int checkCurrItemsAfterShutdown(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1,
 
     checkeq(0, get_int_stat(h, h1, "ep_total_persisted"),
             "Incorrect ep_total_persisted, expected 0");
-    std::stringstream ss;
-    ss << "Incorrect curr_items, expected " << numItems2Load;
-    std::string errmsg(ss.str());
-    checkeq(numItems2Load, get_int_stat(h, h1, "curr_items"),
-            errmsg.c_str());
+
+    // Can only check curr_items in value_only eviction; full-eviction
+    // relies on persistence to complete (via flusher) to update count.
+    const auto evictionPolicy = get_str_stat(h, h1, "ep_item_eviction_policy");
+    if (evictionPolicy == "value_only") {
+        checkeq(numItems2Load,
+                get_int_stat(h, h1, "curr_items"),
+                "Expected curr_items to reflect item count");
+    }
 
     // resume flusher before shutdown + warmup
     pkt = createPacket(PROTOCOL_BINARY_CMD_START_PERSISTENCE);
diff --git a/engines/ep/tests/ep_testsuite_dcp.cc b/engines/ep/tests/ep_testsuite_dcp.cc
@@ -2034,7 +2034,6 @@ static enum test_result test_dcp_producer_stream_req_backfill(ENGINE_HANDLE *h,
         write_items(h, h1, batch_items, start_seqno);
     }
 
-    verify_curr_items(h, h1, num_items, "Wrong amount of items");
     wait_for_stat_to_be_gte(h, h1, "vb_0:num_checkpoints", 2, "checkpoint");
 
     const void *cookie = testHarness.create_cookie();
@@ -4413,14 +4412,11 @@ static enum test_result test_dcp_consumer_mutate(ENGINE_HANDLE *h,
 
 static enum test_result test_dcp_consumer_delete(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1) {
     // Store an item
-    item *i = NULL;
     checkeq(ENGINE_SUCCESS,
-            store(h, h1, NULL, OPERATION_ADD,"key", "value", &i),
+            store(h, h1, NULL, OPERATION_ADD, "key", "value", nullptr),
             "Failed to fail to store an item.");
-    h1->release(h, NULL, i);
-    verify_curr_items(h, h1, 1, "one item stored");
-
     wait_for_flusher_to_settle(h, h1);
+    verify_curr_items(h, h1, 1, "one item stored");
 
     check(set_vbucket_state(h, h1, 0, vbucket_state_replica),
           "Failed to set vbucket state.");
diff --git a/engines/ep/tests/ep_testsuite_xdcr.cc b/engines/ep/tests/ep_testsuite_xdcr.cc
@@ -296,6 +296,7 @@ static enum test_result test_get_meta_with_set(ENGINE_HANDLE *h, ENGINE_HANDLE_V
     checkeq(ENGINE_SUCCESS,
             store(h, h1, NULL, OPERATION_SET, key1, "someothervalue", &i),
             "Failed set.");
+    wait_for_flusher_to_settle(h, h1);
 
     checkeq(1, get_int_stat(h, h1, "curr_items"), "Expected single curr_items");
     checkeq(0, get_int_stat(h, h1, "curr_temp_items"), "Expected zero temp_items");
@@ -944,6 +945,7 @@ static enum test_result test_set_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h
     checkeq(ENGINE_SUCCESS,
             store(h, h1, NULL, OPERATION_SET, key, val, &i),
             "Failed set.");
+    wait_for_flusher_to_settle(h, h1);
 
     // get metadata for the key
     check(get_meta(h, h1, key), "Expected to get meta");
@@ -1119,6 +1121,8 @@ static enum test_result test_set_with_meta_deleted(ENGINE_HANDLE *h, ENGINE_HAND
     // do set with meta with the correct cas value. should pass.
     set_with_meta(h, h1, key, keylen, newVal, newValLen, 0, &itm_meta, cas_for_set);
     checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(), "Expected success");
+    wait_for_flusher_to_settle(h, h1);
+
     // check the stat
     checkeq(1, get_int_stat(h, h1, "ep_num_ops_set_meta"), "Expect some ops");
     checkeq(0, get_int_stat(h, h1, "ep_num_ops_get_meta_on_set_meta"),
@@ -1172,6 +1176,8 @@ static enum test_result test_set_with_meta_nonexistent(ENGINE_HANDLE *h, ENGINE_
     // do set with meta with the correct cas value. should pass.
     set_with_meta(h, h1, key, keylen, val, valLen, 0, &itm_meta, cas_for_set);
     checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(), "Expected success");
+    wait_for_flusher_to_settle(h, h1);
+
     // check the stat
     checkeq(1, get_int_stat(h, h1, "ep_num_ops_set_meta"), "Expect some ops");
     checkeq(1, get_int_stat(h, h1, "curr_items"), "Expected single curr_items");
diff --git a/engines/ep/tests/module_tests/hash_table_test.cc b/engines/ep/tests/module_tests/hash_table_test.cc
@@ -420,15 +420,19 @@ TEST_P(HashTableStatsTest, EjectFlush) {
     v->markClean();
     EXPECT_TRUE(ht.unlocked_ejectItem(v, evictionPolicy));
 
-    EXPECT_EQ(1, ht.getNumItems());
-    if (evictionPolicy == VALUE_ONLY) {
+    switch (evictionPolicy) {
+    case VALUE_ONLY:
+        EXPECT_EQ(1, ht.getNumItems());
         EXPECT_EQ(1, ht.getNumInMemoryItems());
         EXPECT_EQ(1, ht.getNumInMemoryNonResItems());
-    } else {
+        break;
+    case FULL_EVICTION:
         // In full-eviction, ejectItem() also ejects the metadata; hence will
-        // have 0 items in memory.
+        // have 0 items in HashTable.
+        EXPECT_EQ(0, ht.getNumItems());
         EXPECT_EQ(0, ht.getNumInMemoryItems());
         EXPECT_EQ(0, ht.getNumInMemoryNonResItems());
+        break;
     }
     EXPECT_EQ(0, ht.getNumTempItems());
     EXPECT_EQ(0, ht.getNumDeletedItems());
diff --git a/engines/ep/tests/module_tests/vbucket_test.cc b/engines/ep/tests/module_tests/vbucket_test.cc
@@ -517,7 +517,17 @@ TEST_P(VBucketEvictionTest, EjectionResidentCount) {
     EXPECT_EQ(MutationStatus::WasClean,
               this->public_processSet(item, item.getCas()));
 
-    EXPECT_EQ(1, this->vbucket->getNumItems());
+    switch (eviction_policy) {
+    case VALUE_ONLY:
+        // We have accurate VBucket counts in value eviction:
+        EXPECT_EQ(1, this->vbucket->getNumItems());
+        break;
+    case FULL_EVICTION:
+        // In Full Eviction the vBucket count isn't accurate until the
+        // flusher completes - hence just check count in HashTable.
+        EXPECT_EQ(1, this->vbucket->ht.getNumItems());
+        break;
+    }
     EXPECT_EQ(0, this->vbucket->getNumNonResidentItems());
 
     // TODO-MT: Should acquire lock really (ok given this is currently
@@ -530,10 +540,19 @@ TEST_P(VBucketEvictionTest, EjectionResidentCount) {
     EXPECT_TRUE(this->vbucket->ht.unlocked_ejectItem(stored_item,
                                                      eviction_policy));
 
-    // After ejection, should still have 1 item in VBucket, but also have
-    // 1 non-resident item.
-    EXPECT_EQ(1, this->vbucket->getNumItems());
-    EXPECT_EQ(1, this->vbucket->getNumNonResidentItems());
+    switch (eviction_policy) {
+    case VALUE_ONLY:
+        // After ejection, should still have 1 item in HashTable (meta-only),
+        // and VBucket count should also be 1.
+        EXPECT_EQ(1, this->vbucket->getNumItems());
+        EXPECT_EQ(1, this->vbucket->getNumNonResidentItems());
+        break;
+    case FULL_EVICTION:
+        // In Full eviction should be no items in HashTable (we fully
+        // evicted it).
+        EXPECT_EQ(0, this->vbucket->ht.getNumItems());
+        break;
+    }
 }
 
 // Regression test for MB-21448 - if an attempt is made to perform a CAS
