Revert "MB-34017 [SR]: Include the prepared-seqno in Commit queued items"

Conflicts with 233945f. Reverting while it is re-worked.

This reverts commit c7e524a.

Change-Id: I8f1cdcc0ff4591fe86c083584438e5fb2c5814fa
Reviewed-on: http://review.couchbase.org/109986
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Dave Rigby <[email protected]>

Author: daverigby

engines/ep/src/item.h

@@ -515,10 +515,10 @@ class Item : public RCValue {
     std::atomic<int64_t> bySeqno;
 
     // @todo: Try to avoid this, as it increases mem_usage of 8 bytes per Item.
-    // This is added for Durability items Commit and Abort, for which need the
-    // associated Prepare's seqno at persistence for writing the correct High
-    // Completed Seqno (HCS) to disk. Note that the HCS is the seqno of the
-    // last Committed or Aborted Prepare.
+    // This is added for Durability items Commit and Abort, which both carry
+    // 2 seqnos:
+    // 1) the seqno of this (Commit/Abort) Item (encoded in bySeqno)
+    // 2) the seqno of the Committed/Aborted Prepare
     cb::uint48_t prepareSeqno;
 
     uint32_t queuedTime;

engines/ep/src/vbucket.cc

@@ -798,9 +798,6 @@ ENGINE_ERROR_CODE VBucket::commit(
     // it may have been set explicitly.
     queueItmCtx.genCas = GenerateCas::No;
 
-    queueItmCtx.durability =
-            DurabilityItemCtx{res.pending->getBySeqno(), nullptr /*cookie*/};
-
     auto notify = commitStoredValue(res, queueItmCtx, commitSeqno);
 
     notifyNewSeqno(notify);
@@ -1148,26 +1145,17 @@ VBNotifyCtx VBucket::queueDirty(const HashTable::HashBucketLock& hbl,
         setMaxCasAndTrackDrift(v.getCas());
     }
 
-    // If we are queueing a SyncWrite StoredValue; extract the durability
+    // If we are queuing a SyncWrite StoredValue; extract the durability
     // requirements to use to create the Item.
     boost::optional<cb::durability::Requirements> durabilityReqs;
-    if (v.isPending()) {
-        Expects(ctx.durability.is_initialized());
-        durabilityReqs = boost::get<cb::durability::Requirements>(
-                ctx.durability->requirementsOrPreparedSeqno);
+    if (ctx.durability) {
+        durabilityReqs = ctx.durability->requirements;
     }
-
     queued_item qi(v.toItem(getId(),
                             StoredValue::HideLockedCas::No,
                             StoredValue::IncludeValue::Yes,
                             durabilityReqs));
 
-    if (qi->isCommitSyncWrite()) {
-        Expects(ctx.durability.is_initialized());
-        qi->setPrepareSeqno(boost::get<int64_t>(
-                ctx.durability->requirementsOrPreparedSeqno));
-    }
-
     // MB-27457: Timestamp deletes only when they don't already have a timestamp
     // assigned. This is here to ensure all deleted items have a timestamp which
     // our tombstone purger can use to determine which tombstones to purge. A

engines/ep/src/vbucket.h

@@ -28,8 +28,6 @@
 #include "vbucket_bgfetch_item.h"
 #include "vbucket_fwd.h"
 
-#include "boost/variant.hpp"
-
 #include <folly/Synchronized.h>
 #include <memcached/engine.h>
 #include <nlohmann/json.hpp>
@@ -80,13 +78,8 @@ struct VBNotifyCtx {
  * has Durability requirements.
  */
 struct DurabilityItemCtx {
-    /**
-     * Stores:
-     * - the Durability Requirements, if we queue a Prepare
-     * - the prepared-seqno, if we queue a Commit
-     */
-    boost::variant<cb::durability::Requirements, int64_t>
-            requirementsOrPreparedSeqno;
+    /// The durability requirements for this item.
+    cb::durability::Requirements requirements;
     /**
      * The client cookie associated with the Durability operation. If non-null
      * then notifyIOComplete will be called on it when operation is committed.
@@ -1802,7 +1795,6 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
      *
      * @param hbl The lock to the HashTable-bucket containing the StoredValue
      * @param v The StoredValue of the Pending being aborted.
-     * @param prepareSeqno The seqno of the Prepare being aborted
      * @param ctx The VBQueueItemCtx. Holds info needed to enqueue the item,
      *     look at the structure for details.
      * @return the notification context used for notifying the Flusher and

engines/ep/tests/module_tests/vbucket_durability_test.cc

@@ -491,7 +491,7 @@ TEST_P(VBucketDurabilityTest, Active_Commit_MultipleReplicas) {
             CheckpointManagerTestIntrospector::public_getCheckpointList(
                     *ckptMgr);
 
-    auto checkPending = [this, &key, &ckptList, preparedSeqno]() -> void {
+    auto checkPending = [this, &key, &ckptList]() -> void {
         EXPECT_EQ(nullptr, ht->findForRead(key).storedValue);
         const auto sv = ht->findForWrite(key).storedValue;
         ASSERT_NE(nullptr, sv);
@@ -501,13 +501,11 @@ TEST_P(VBucketDurabilityTest, Active_Commit_MultipleReplicas) {
         for (const auto& qi : *ckptList.front()) {
             if (!qi->isCheckPointMetaItem()) {
                 EXPECT_EQ(queue_op::pending_sync_write, qi->getOperation());
-                EXPECT_EQ(preparedSeqno, qi->getBySeqno());
-                EXPECT_EQ("value", qi->getValue()->to_s());
             }
         }
     };
 
-    auto checkCommitted = [this, &key, &ckptList, preparedSeqno]() -> void {
+    auto checkCommitted = [this, &key, &ckptList]() -> void {
         const auto sv = ht->findForRead(key).storedValue;
         ASSERT_NE(nullptr, sv);
         EXPECT_NE(nullptr, ht->findForWrite(key).storedValue);
@@ -517,9 +515,6 @@ TEST_P(VBucketDurabilityTest, Active_Commit_MultipleReplicas) {
         for (const auto& qi : *ckptList.front()) {
             if (!qi->isCheckPointMetaItem()) {
                 EXPECT_EQ(queue_op::commit_sync_write, qi->getOperation());
-                EXPECT_GT(qi->getBySeqno() /*commitSeqno*/, preparedSeqno);
-                EXPECT_EQ(preparedSeqno, qi->getPrepareSeqno());
-                EXPECT_EQ("value", qi->getValue()->to_s());
             }
         }
     };
@@ -684,8 +679,7 @@ TEST_P(VBucketDurabilityTest, NonPendingKeyAtAbort) {
  * 3) the abort_sync_write is not added to the DurabilityMonitor
  */
 TEST_P(VBucketDurabilityTest, Active_AbortSyncWrite) {
-    const int64_t preparedSeqno = 1;
-    storeSyncWrites({preparedSeqno});
+    storeSyncWrites({1} /*seqno*/);
     ASSERT_EQ(1,
               VBucketTestIntrospector::public_getActiveDM(*vbucket)
                       .getNumTracked());
@@ -701,9 +695,9 @@ TEST_P(VBucketDurabilityTest, Active_AbortSyncWrite) {
         // Visible at write
         auto storedItem = ht->findForWrite(key);
         ASSERT_TRUE(storedItem.storedValue);
+        // item pending
         EXPECT_EQ(CommittedState::Pending,
                   storedItem.storedValue->getCommitted());
-        EXPECT_EQ(preparedSeqno, storedItem.storedValue->getBySeqno());
     }
 
     const auto& ckptList =
@@ -725,7 +719,6 @@ TEST_P(VBucketDurabilityTest, Active_AbortSyncWrite) {
     it++;
     ASSERT_EQ(1, ckpt->getNumItems());
     EXPECT_EQ(queue_op::pending_sync_write, (*it)->getOperation());
-    EXPECT_EQ(preparedSeqno, (*it)->getBySeqno());
     EXPECT_EQ("value", (*it)->getValue()->to_s());
 
     // The Pending is tracked by the DurabilityMonitor
@@ -772,8 +765,6 @@ TEST_P(VBucketDurabilityTest, Active_AbortSyncWrite) {
     EXPECT_EQ(queue_op::abort_sync_write, (*it)->getOperation());
     EXPECT_TRUE((*it)->isDeleted());
     EXPECT_FALSE((*it)->getValue());
-    EXPECT_GT((*it)->getBySeqno(), preparedSeqno);
-    EXPECT_EQ(preparedSeqno, (*it)->getPrepareSeqno());
 
     // The Aborted item is not added for tracking.
     // Note: The Pending has not been removed as we are testing at VBucket
@@ -1035,9 +1026,8 @@ void VBucketDurabilityTest::testHTSyncDeleteCommit() {
     auto* writeView = ht->findForWrite(key).storedValue;
     ASSERT_TRUE(writeView);
     VBQueueItemCtx ctx;
-    ctx.durability = DurabilityItemCtx{
-            cb::durability::Requirements{cb::durability::Level::Majority, {}},
-            cookie};
+    ctx.durability =
+            DurabilityItemCtx{{cb::durability::Level::Majority, {}}, cookie};
     ASSERT_EQ(MutationStatus::WasDirty,
               public_processSoftDelete(key, ctx).first);
 
@@ -1472,28 +1462,14 @@ void VBucketDurabilityTest::testCompleteSWInPassiveDM(vbucket_state_t state,
     const auto& ckptList =
             CheckpointManagerTestIntrospector::public_getCheckpointList(
                     *ckptMgr);
-    // 1 checkpoint
     ASSERT_EQ(1, ckptList.size());
-    // empty-item
-    const auto& ckpt = *ckptList.front();
-    auto it = ckpt.begin();
-    ASSERT_EQ(queue_op::empty, (*it)->getOperation());
-    // 1 metaitem (checkpoint-start)
-    it++;
-    ASSERT_EQ(1, ckpt.getNumMetaItems());
-    EXPECT_EQ(queue_op::checkpoint_start, (*it)->getOperation());
-    // 3 non-metaitem are Committed or Aborted
-    ASSERT_EQ(writes.size(), ckpt.getNumItems());
+    EXPECT_EQ(writes.size(), ckptList.front()->getNumItems());
     const auto expectedOp =
             (res == Resolution::Commit ? queue_op::commit_sync_write
                                        : queue_op::abort_sync_write);
-    for (const auto& prepare : writes) {
-        it++;
-        EXPECT_EQ(expectedOp, (*it)->getOperation());
-        EXPECT_GT((*it)->getBySeqno() /*commitSeqno*/, prepare.seqno);
-        EXPECT_EQ(prepare.seqno, (*it)->getPrepareSeqno());
-        if (expectedOp == queue_op::commit_sync_write) {
-            EXPECT_EQ("value", (*it)->getValue()->to_s());
+    for (const auto& qi : *ckptList.front()) {
+        if (!qi->isCheckPointMetaItem()) {
+            EXPECT_EQ(expectedOp, qi->getOperation());
         }
     }