Fix DocKey::to_string() parsing of queued_item's keys

rdemellow · rdemellow · commit a43ff3faf55d · 2020-11-30T16:03:38.000Z
Currently we don't parse the key of a empty, checkpoint_start, set_vbucket_sate and checkpoint_end keys correctly. For example the below checkpoint dump shows that we crop the first few chars and interpret them as leb128 values: {2803,empty,cid:0x1:0x65:0x6d:pty,118,[m]} {2803,checkpoint_start,cid:0x1:0x63:0x68:eckpoint_start,129,[m]} {2803,system_event[d],cid:0x1:0x0:0x385:_collection,176,} {2804,set_vbucket_state,cid:0x1:0x73:0x65:t_vbucket_state,241,[m]} {2804,checkpoint_end,cid:0x1:0x63:0x68:eckpoint_end,127,[m]} With this patch: {2803,empty,cid:0x1:empty,118,[m]} {2803,checkpoint_start,cid:0x1:checkpoint_start,129,[m]} {2803,system_event[d],cid:0x1:0x0:0x385:_collection,176,} {2804,set_vbucket_state,cid:0x1:set_vbucket_state,241,[m]} {2804,checkpoint_end,cid:0x1:checkpoint_end,127,[m]} To also ensure we parse system events correctly, this patch moves enum Class SystemEvent to include/memcached/systemevent.h so that it can be accessed by dockey.cc. This patch also clang-tidys some lines in checkpoint_test.cc and also adds test to ensure correct parsing of all types of items. Change-Id: I31db2826316f60b068f7e9370e488f0df0ae4324 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/141044 Reviewed-by: Dave Rigby <daver@couchbase.com> Tested-by: Build Bot <build@couchbase.com>
diff --git a/engines/ep/CMakeLists.txt b/engines/ep/CMakeLists.txt
@@ -357,7 +357,7 @@ ADD_LIBRARY(ep_objs OBJECT
             src/stored-value.cc
             src/stored_value_factories.cc
             src/stored_value_factories.h
-            src/systemevent.cc
+            src/systemevent_factory.cc
             src/tasks.cc
             src/taskqueue.cc
             src/vb_commit.cc
diff --git a/engines/ep/src/collections/collections_types.cc b/engines/ep/src/collections/collections_types.cc
@@ -16,7 +16,7 @@
  */
 
 #include "collections/collections_types.h"
-#include "systemevent.h"
+#include "systemevent_factory.h"
 
 #include <mcbp/protocol/unsigned_leb128.h>
 #include <nlohmann/json.hpp>
diff --git a/engines/ep/src/collections/eraser_context.h b/engines/ep/src/collections/eraser_context.h
@@ -16,7 +16,7 @@
  */
 
 #include "collections/scan_context.h"
-#include "systemevent.h"
+#include "systemevent_factory.h"
 
 #pragma once
 
diff --git a/engines/ep/src/collections/vbucket_manifest.cc b/engines/ep/src/collections/vbucket_manifest.cc
@@ -25,7 +25,7 @@
 #include "collections/manifest.h"
 #include "ep_time.h"
 #include "item.h"
-#include "systemevent.h"
+#include "systemevent_factory.h"
 #include "vbucket.h"
 
 #include <statistics/cbstat_collector.h>
diff --git a/engines/ep/src/dcp/response.h b/engines/ep/src/dcp/response.h
@@ -21,7 +21,7 @@
 #include "ep_types.h"
 #include "ext_meta_parser.h"
 #include "item.h"
-#include "systemevent.h"
+#include "systemevent_factory.h"
 
 #include <mcbp/protocol/dcp_stream_end_status.h>
 #include <memcached/dcp_stream_id.h>
diff --git a/engines/ep/src/stored-value.cc b/engines/ep/src/stored-value.cc
@@ -21,7 +21,7 @@
 #include "item.h"
 #include "objectregistry.h"
 #include "stats.h"
-#include "systemevent.h"
+#include "systemevent_factory.h"
 
 #include <platform/cb_malloc.h>
 #include <platform/compress.h>
diff --git a/engines/ep/src/systemevent_factory.cc b/engines/ep/src/systemevent_factory.cc
@@ -15,7 +15,7 @@
  *   limitations under the License.
  */
 
-#include "systemevent.h"
+#include "systemevent_factory.h"
 
 #include "collections/collections_types.h"
 #include "collections/vbucket_manifest.h"
diff --git a/engines/ep/src/systemevent_factory.h b/engines/ep/src/systemevent_factory.h
@@ -22,6 +22,8 @@
 #include "ep_types.h"
 #include "storeddockey_fwd.h"
 
+#include <memcached/systemevent.h>
+
 #include <string>
 
 class Item;
@@ -32,46 +34,6 @@ namespace Collections::VB {
 class Manifest;
 } // namespace Collections::VB
 
-/// underlying size of uint32_t as this is to be stored in the Item flags field.
-enum class SystemEvent : uint32_t {
-    /**
-     * The Collection system event represents the beginning or end of a
-     * collection. Each Collection system event has a key which contains the
-     * collection ID. When the event is queued in a checkpoint or stored on
-     * disk the seqno of that item states that this is the point when that
-     * collection became accessible unless that queued/stored item is deleted,
-     * then it represent when that collection became inaccesible (logically
-     * deleted).
-     *
-     * A Collection system event when queued into a checkpoint carries with it
-     * a value, the value is used to maintain a per vbucket JSON collection's
-     * manifest (for persisted buckets).
-     */
-    Collection,
-
-    /**
-     * The Scope system event represents the beginning or end of a
-     * scope. Each Scope system event has a key which contains the
-     * Scope ID. When the event is queued in a checkpoint or stored on
-     * disk the seqno of that item states that this is the point when that
-     * scope became accessible unless that queued/stored item is deleted,
-     * then it represent when that scope became inaccessible
-     *
-     */
-    Scope
-};
-
-static inline std::string to_string(const SystemEvent se) {
-    switch (se) {
-    case SystemEvent::Collection:
-        return "Collection";
-    case SystemEvent::Scope:
-        return "Scope";
-    }
-    throw std::invalid_argument("to_string(SystemEvent) unknown " +
-                                std::to_string(int(se)));
-}
-
 class SystemEventFactory {
 public:
     /**
diff --git a/engines/ep/tests/module_tests/checkpoint_test.cc b/engines/ep/tests/module_tests/checkpoint_test.cc
@@ -33,9 +33,7 @@
 #include "../mock/mock_checkpoint_manager.h"
 #include "../mock/mock_dcp_consumer.h"
 #include "../mock/mock_stream.h"
-#include "../mock/mock_synchronous_ep_engine.h"
 
-#include <folly/portability/GMock.h>
 #include <folly/portability/GTest.h>
 #include <valgrind/valgrind.h>
 
@@ -47,7 +45,6 @@
 #define NUM_SET_THREADS_VG 2
 
 #define NUM_ITEMS 500
-#define NUM_ITEMS_VG 10
 
 #define DCP_CURSOR_PREFIX "dcp-client-"
 
@@ -1024,7 +1021,7 @@ TEST_P(CheckpointTest, SeqnoAndHLCOrdering) {
     std::vector<std::vector<std::pair<uint64_t, uint64_t> > > threadData(n_threads);
     for (int ii = 0; ii < n_threads; ii++) {
         auto& threadsData = threadData[ii];
-        threads.push_back(std::thread([this, ii, n_items, &threadsData](){
+        threads.emplace_back([this, ii, n_items, &threadsData]() {
             std::string key = "key" + std::to_string(ii);
             for (int item  = 0; item < n_items; item++) {
                 queued_item qi(
@@ -1041,9 +1038,9 @@ TEST_P(CheckpointTest, SeqnoAndHLCOrdering) {
                                                   /*preLinkDocCtx*/ nullptr));
 
                 // Save seqno/cas
-                threadsData.push_back(std::make_pair(qi->getBySeqno(), qi->getCas()));
+                threadsData.emplace_back(qi->getBySeqno(), qi->getCas());
             }
-        }));
+        });
     }
 
     // Wait for all threads
@@ -2984,6 +2981,36 @@ TEST_P(CheckpointTest, MB_41283_TestNoCrashForDuplicateKeysInDiskCheckpoint) {
     EXPECT_FALSE(queueReplicatedItem("k1001", 1002));
 }
 
+TEST_P(CheckpointTest, CheckpointItemToString) {
+    auto item = this->manager->public_createCheckpointItem(
+            0, Vbid(0), queue_op::empty);
+    EXPECT_EQ("cid:0x1:empty", item->getKey().to_string());
+
+    item = this->manager->public_createCheckpointItem(
+            0, Vbid(0), queue_op::checkpoint_start);
+    EXPECT_EQ("cid:0x1:checkpoint_start", item->getKey().to_string());
+
+    item = this->manager->public_createCheckpointItem(
+            0, Vbid(0), queue_op::set_vbucket_state);
+    EXPECT_EQ("cid:0x1:set_vbucket_state", item->getKey().to_string());
+
+    item = this->manager->public_createCheckpointItem(
+            0, Vbid(0), queue_op::checkpoint_end);
+    EXPECT_EQ("cid:0x1:checkpoint_end", item->getKey().to_string());
+
+    auto disk = makeDiskDocKey("test_key");
+    EXPECT_EQ("cid:0x0:test_key", disk.to_string());
+
+    disk = makeDiskDocKey("test_key", true, CollectionID(99));
+    EXPECT_EQ("pre:cid:0x63:test_key", disk.to_string());
+
+    auto event =
+            SystemEventFactory::makeCollectionEvent(CollectionID(99), {}, {});
+    EXPECT_EQ("cid:0x1:0x0:0x63:_collection", event->getKey().to_string());
+    event = SystemEventFactory::makeScopeEvent(ScopeID(99), {}, {});
+    EXPECT_EQ("cid:0x1:0x1:0x63:_scope", event->getKey().to_string());
+}
+
 INSTANTIATE_TEST_SUITE_P(
         AllVBTypesAllEvictionModes,
         CheckpointTest,
diff --git a/engines/ep/tests/module_tests/collections/vbucket_manifest_test.cc b/engines/ep/tests/module_tests/collections/vbucket_manifest_test.cc
@@ -25,7 +25,7 @@
 #include "failover-table.h"
 #include "item.h"
 #include "stats.h"
-#include "systemevent.h"
+#include "systemevent_factory.h"
 #include "tests/module_tests/collections/collections_test_helpers.h"
 #include "tests/module_tests/test_helpers.h"
 #include <utilities/test_manifest.h>
diff --git a/engines/ep/tests/module_tests/systemevent_test.cc b/engines/ep/tests/module_tests/systemevent_test.cc
@@ -16,7 +16,7 @@
  */
 
 #include "item.h"
-#include "systemevent.h"
+#include "systemevent_factory.h"
 
 #include <folly/portability/GTest.h>
 
diff --git a/include/memcached/systemevent.h b/include/memcached/systemevent.h
@@ -0,0 +1,62 @@
+/* -*- Mode: C++; tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+ *     Copyright 2020 Couchbase, Inc
+ *
+ *   Licensed under the Apache License, Version 2.0 (the "License");
+ *   you may not use this file except in compliance with the License.
+ *   You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   Unless required by applicable law or agreed to in writing, software
+ *   distributed under the License is distributed on an "AS IS" BASIS,
+ *   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *   See the License for the specific language governing permissions and
+ *   limitations under the License.
+ */
+
+#include <cinttypes>
+#include <stdexcept>
+#include <string>
+
+#pragma once
+
+/// underlying size of uint32_t as this is to be stored in the Item flags field.
+enum class SystemEvent : uint32_t {
+    /**
+     * The Collection system event represents the beginning or end of a
+     * collection. Each Collection system event has a key which contains the
+     * collection ID. When the event is queued in a checkpoint or stored on
+     * disk the seqno of that item states that this is the point when that
+     * collection became accessible unless that queued/stored item is deleted,
+     * then it represent when that collection became inaccesible (logically
+     * deleted).
+     *
+     * A Collection system event when queued into a checkpoint carries with it
+     * a value, the value is used to maintain a per vbucket JSON collection's
+     * manifest (for persisted buckets).
+     */
+    Collection,
+
+    /**
+     * The Scope system event represents the beginning or end of a
+     * scope. Each Scope system event has a key which contains the
+     * Scope ID. When the event is queued in a checkpoint or stored on
+     * disk the seqno of that item states that this is the point when that
+     * scope became accessible unless that queued/stored item is deleted,
+     * then it represent when that scope became inaccessible
+     *
+     */
+    Scope
+};
+
+static inline std::string to_string(const SystemEvent se) {
+    switch (se) {
+    case SystemEvent::Collection:
+        return "Collection";
+    case SystemEvent::Scope:
+        return "Scope";
+    }
+    throw std::invalid_argument("to_string(SystemEvent) unknown " +
+                                std::to_string(int(se)));
+}
diff --git a/utilities/dockey.cc b/utilities/dockey.cc
@@ -17,6 +17,7 @@
 
 #include <mcbp/protocol/unsigned_leb128.h>
 #include <memcached/dockey.h>
+#include <memcached/systemevent.h>
 #include <spdlog/fmt/fmt.h>
 #include <ostream>
 #include <sstream>
@@ -66,17 +67,19 @@ std::string DocKey::to_string() const {
         auto [cidOrSid, keySuffix] =
                 cb::mcbp::unsigned_leb128<uint32_t>::decode(
                         {keyWithoutEvent.data(), keyWithoutEvent.size()});
-        // Get the string view to the remaining string part of the key
-        remainingKey = {reinterpret_cast<const char*>(keySuffix.data()),
-                        keySuffix.size()};
-        return fmt::format(fmt("cid:{:#x}:{:#x}:{:#x}:{}"),
-                           cid,
-                           systemEventID,
-                           cidOrSid,
-                           remainingKey);
-    } else {
-        return fmt::format(fmt("cid:{:#x}:{}"), cid, remainingKey);
+        if (systemEventID == uint32_t(SystemEvent::Collection) ||
+            systemEventID == uint32_t(SystemEvent::Scope)) {
+            // Get the string view to the remaining string part of the key
+            remainingKey = {reinterpret_cast<const char*>(keySuffix.data()),
+                            keySuffix.size()};
+            return fmt::format(fmt("cid:{:#x}:{:#x}:{:#x}:{}"),
+                               cid,
+                               systemEventID,
+                               cidOrSid,
+                               remainingKey);
+        }
     }
+    return fmt::format(fmt("cid:{:#x}:{}"), cid, remainingKey);
 }
 
 CollectionID DocKey::getCollectionID() const {
