MB-57609: Do not increment numGetFailure for document not found

The kv_ep_data_read_failed stat is used by ns_server to detect sustained
disk errors and fail over the node.

Do not increment the stat when the document does not exist.

Change-Id: I7847528106a42dfcc6255e7d943e68a2131b6653
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/193173
Tested-by: Build Bot <[email protected]>
Reviewed-by: Jim Walker <[email protected]>
Reviewed-by: Paolo Cocchi <[email protected]>
Reviewed-by: James H <[email protected]>
Well-Formed: Restriction Checker

Author: veselink1

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

@@ -639,7 +639,9 @@ GetValue CouchKVStore::getWithHeader(DbHolder& db,
         }
     }
 
-    if(errCode != COUCHSTORE_SUCCESS) {
+    // MB-57609: Only report actual get errors, no_such_key should not count
+    if (errCode != COUCHSTORE_SUCCESS &&
+        errCode != COUCHSTORE_ERROR_DOC_NOT_FOUND) {
         ++st.numGetFailure;
     }
 

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

@@ -954,9 +954,6 @@ GetValue MagmaKVStore::getWithHeader(const DiskDocKey& key,
     }
 
     if (!found) {
-        // Whilst this isn't strictly a failure if we're running full eviction
-        // it could be considered one for value eviction.
-        st.numGetFailure++;
         return GetValue{nullptr, cb::engine_errc::no_such_key};
     }
 

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

@@ -697,7 +697,9 @@ GetValue RocksDBKVStore::getWithHeader(const DiskDocKey& key,
     rocksdb::Status s = rdb->Get(
             rocksdb::ReadOptions(), vbh->defaultCFH.get(), keySlice, &value);
     if (!s.ok()) {
-        st.numGetFailure++;
+        if (!s.IsNotFound()) {
+            st.numGetFailure++;
+        }
         return GetValue{nullptr, cb::engine_errc::no_such_key};
     }
 

engines/ep/tests/module_tests/couch-kvstore_test.cc

@@ -830,6 +830,11 @@ TEST_F(CouchKVStoreErrorInjectionTest, get_docinfo_by_id) {
         gv = kvstore->get(DiskDocKey{*items.front()}, Vbid(0));
     }
     EXPECT_EQ(cb::engine_errc::temporary_failure, gv.getStatus());
+
+    /* Check we incremented the read failures */
+    size_t getFailures = 0;
+    kvstore->getStat("failure_get", getFailures);
+    EXPECT_EQ(1, getFailures);
 }
 
 /**
@@ -881,6 +886,11 @@ TEST_F(CouchKVStoreErrorInjectionTest, getMulti_docinfos_by_id) {
     }
     EXPECT_EQ(cb::engine_errc::temporary_failure,
               itms[DiskDocKey{*items.at(0)}].value.getStatus());
+
+    /* Check we incremented the read failures */
+    size_t getFailures = 0;
+    kvstore->getStat("failure_get", getFailures);
+    EXPECT_EQ(1, getFailures);
 }
 
 /**

engines/ep/tests/module_tests/kvstore_test.cc

@@ -365,14 +365,14 @@ TEST_P(KVStoreParamTest, GetModes) {
     EXPECT_FALSE(gv.item->getValue());
 }
 
-// A doc not found should equal a get failure for a get call (used for some
-// stats, fetching docs to expire, and rollback)
+// A doc not found should not equal a get failure for a get call (used for
+// failover by ns_server).
 TEST_P(KVStoreParamTest, GetMissNumGetFailure) {
     GetValue gv = kvstore->get(DiskDocKey{makeStoredDocKey("key")}, Vbid(0));
     EXPECT_EQ(cb::engine_errc::no_such_key, gv.getStatus());
 
     auto stats = kvstore->getKVStoreStat();
-    EXPECT_EQ(1, stats.numGetFailure);
+    EXPECT_EQ(0, stats.numGetFailure);
     EXPECT_EQ(0, kvstore->getKVStoreStat().io_bg_fetch_docs_read);
     EXPECT_EQ(0, kvstore->getKVStoreStat().io_bgfetch_doc_bytes);
 }