MB-20852 [7/N]: CheckpointManager::queueDirty: Pass vb by reference 16/69016/5
authorDave Rigby <daver@couchbase.com>
Wed, 19 Oct 2016 10:26:33 +0000 (11:26 +0100)
committerDave Rigby <daver@couchbase.com>
Mon, 31 Oct 2016 15:36:27 +0000 (15:36 +0000)
The VBucket argument to queueDirty is passed via a (reference to) a
ref-counted pointer. Nothing in this function modifies the arguments'
ref count, or passes it another owner; moreover it is also never null.

Therefore change to pass as a reference.

See also:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-smartptrparam

Change-Id: Iccfeb42922da558b6e9ab430b96829002e85af4a
Reviewed-on: http://review.couchbase.org/69016
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
src/checkpoint.cc
src/checkpoint.h
src/ep.cc
src/ep.h
tests/module_tests/checkpoint_test.cc

index f456bac..b069ba1 100644 (file)
@@ -976,15 +976,11 @@ std::vector<std::string> CheckpointManager::getListOfCursorsToDrop() {
     return cursorsToDrop;
 }
 
-bool CheckpointManager::queueDirty(const RCPtr<VBucket> &vb,
-                                   queued_item& qi,
+bool CheckpointManager::queueDirty(VBucket& vb, queued_item& qi,
                                    const GenerateBySeqno generateBySeqno,
                                    const GenerateCas generateCas) {
     LockHolder lh(queueLock);
-    if (!vb) {
-        throw std::invalid_argument("CheckpointManager::queueDirty: vb must "
-                        "be non-NULL");
-    }
+
     bool canCreateNewCheckpoint = false;
     if (checkpointList.size() < checkpointConfig.getMaxCheckpoints() ||
         (checkpointList.size() == checkpointConfig.getMaxCheckpoints() &&
@@ -992,20 +988,20 @@ bool CheckpointManager::queueDirty(const RCPtr<VBucket> &vb,
         canCreateNewCheckpoint = true;
     }
 
-    if (vb->getState() == vbucket_state_active && canCreateNewCheckpoint) {
+    if (vb.getState() == vbucket_state_active && canCreateNewCheckpoint) {
         // Only the master active vbucket can create a next open checkpoint.
         checkOpenCheckpoint_UNLOCKED(false, true);
     }
 
     if (checkpointList.back()->getState() == CHECKPOINT_CLOSED) {
-        if (vb->getState() == vbucket_state_active) {
+        if (vb.getState() == vbucket_state_active) {
             addNewCheckpoint_UNLOCKED(checkpointList.back()->getId() + 1);
         } else {
             throw std::logic_error("CheckpointManager::queueDirty: vBucket "
                     "state (which is " +
-                    std::string(VBucket::toString(vb->getState())) +
+                    std::string(VBucket::toString(vb.getState())) +
                     ") is not active. This is not expected. vb:" +
-                    std::to_string(vb->getId()) +
+                    std::to_string(vb.getId()) +
                     " lastBySeqno:" + std::to_string(lastBySeqno) +
                     " genSeqno:" + to_string(generateBySeqno));
         }
@@ -1028,7 +1024,7 @@ bool CheckpointManager::queueDirty(const RCPtr<VBucket> &vb,
     // MB-20798: Allow the HLC to be created 'atomically' with the seqno as
     // we're holding the ::queueLock.
     if (GenerateCas::Yes == generateCas) {
-        qi->setCas(vb->nextHLCCas());
+        qi->setCas(vb.nextHLCCas());
     }
 
     uint64_t st = checkpointList.back()->getSnapshotStartSeqno();
@@ -1036,8 +1032,8 @@ bool CheckpointManager::queueDirty(const RCPtr<VBucket> &vb,
     if (!(st <= static_cast<uint64_t>(lastBySeqno) &&
           static_cast<uint64_t>(lastBySeqno) <= en)) {
         throw std::logic_error("CheckpointManager::queueDirty: lastBySeqno "
-                "not in snapshot range. vb:" + std::to_string(vb->getId()) +
-                " state:" + std::string(VBucket::toString(vb->getState())) +
+                "not in snapshot range. vb:" + std::to_string(vb.getId()) +
+                " state:" + std::string(VBucket::toString(vb.getState())) +
                 " snapshotStart:" + std::to_string(st) +
                 " lastBySeqno:" + std::to_string(lastBySeqno) +
                 " snapshotEnd:" + std::to_string(en) +
@@ -1053,7 +1049,7 @@ bool CheckpointManager::queueDirty(const RCPtr<VBucket> &vb,
     if (result != EXISTING_ITEM) {
         ++stats.totalEnqueued;
         ++stats.diskQueueSize;
-        vb->doStatsForQueueing(*qi, qi->size());
+        vb.doStatsForQueueing(*qi, qi->size());
 
         // Update the checkpoint's memory usage
         checkpointList.back()->incrementMemConsumption(qi->size());
index 1233cda..439d62c 100644 (file)
@@ -631,13 +631,12 @@ public:
 
     /**
      * Queue an item to be written to persistent layer.
-     * @param vbucket the vbucket that a new item is pushed into.
+     * @param vb the vbucket that a new item is pushed into.
      * @param qi item to be persisted.
      * @param generateBySeqno yes/no generate the seqno for the item
      * @return true if an item queued increases the size of persistence queue by 1.
      */
-    bool queueDirty(const RCPtr<VBucket> &vb,
-                    queued_item& qi,
+    bool queueDirty(VBucket& vb, queued_item& qi,
                     const GenerateBySeqno generateBySeqno,
                     const GenerateCas generateCas);
 
index 843bc75..73b0c6a 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -1105,7 +1105,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::addTAPBackfillItem(
         // FALLTHROUGH
     case WAS_CLEAN:
         vb->setMaxCas(v->getCas());
-        tapQueueDirty(vb, v, lh, NULL,
+        tapQueueDirty(*vb, v, lh, NULL,
                       genBySeqno ? GenerateBySeqno::Yes : GenerateBySeqno::No);
         break;
     case INVALID_VBUCKET:
@@ -3031,7 +3031,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::deleteWithMeta(
         vb->setMaxCasAndTrackDrift(v->getCas());
 
         if (tapBackfill) {
-            tapQueueDirty(vb, v, lh, seqno,
+            tapQueueDirty(*vb, v, lh, seqno,
                           genBySeqno ? GenerateBySeqno::Yes : GenerateBySeqno::No);
         } else {
             queueDirty(vb, v, &lh, seqno,
@@ -3527,7 +3527,7 @@ void EventuallyPersistentStore::queueDirty(RCPtr<VBucket> &vb,
     if (vb) {
         queued_item qi(v->toItem(false, vb->getId()));
 
-        bool rv = vb->checkpointManager.queueDirty(vb, qi,
+        bool rv = vb->checkpointManager.queueDirty(*vb, qi,
                                                    generateBySeqno, generateCas);
         v->setBySeqno(qi->getBySeqno());
 
@@ -3555,36 +3555,34 @@ void EventuallyPersistentStore::queueDirty(RCPtr<VBucket> &vb,
     }
 }
 
-void EventuallyPersistentStore::tapQueueDirty(RCPtr<VBucket> &vb,
+void EventuallyPersistentStore::tapQueueDirty(VBucket &vb,
                                               StoredValue* v,
                                               LockHolder& plh,
                                               uint64_t *seqno,
                                               const GenerateBySeqno generateBySeqno) {
-    if (vb) {
-        queued_item qi(v->toItem(false, vb->getId()));
+    queued_item qi(v->toItem(false, vb.getId()));
 
-        bool queued = vb->queueBackfillItem(qi, generateBySeqno);
+    bool queued = vb.queueBackfillItem(qi, generateBySeqno);
 
-        v->setBySeqno(qi->getBySeqno());
+    v->setBySeqno(qi->getBySeqno());
 
-        /* During backfill on a TAP receiver we need to update the snapshot
-           range in the checkpoint. Has to be done here because in case of TAP
-           backfill, above, we use vb->queueBackfillItem() instead of
-           vb->checkpointManager.queueDirty() */
-        if (GenerateBySeqno::Yes == generateBySeqno) {
-            vb->checkpointManager.resetSnapshotRange();
-        }
+    /* During backfill on a TAP receiver we need to update the snapshot
+       range in the checkpoint. Has to be done here because in case of TAP
+       backfill, above, we use vb.queueBackfillItem() instead of
+       vb.checkpointManager.queueDirty() */
+    if (GenerateBySeqno::Yes == generateBySeqno) {
+        vb.checkpointManager.resetSnapshotRange();
+    }
 
-        if (seqno) {
-            *seqno = v->getBySeqno();
-        }
+    if (seqno) {
+        *seqno = v->getBySeqno();
+    }
 
-        plh.unlock();
+    plh.unlock();
 
-        if (queued) {
-            KVShard* shard = vbMap.getShardByVbId(vb->getId());
-            shard->getFlusher()->notifyFlushEvent();
-        }
+    if (queued) {
+        KVShard* shard = vbMap.getShardByVbId(vb.getId());
+        shard->getFlusher()->notifyFlushEvent();
     }
 }
 
index ad5acfd..f5163a2 100644 (file)
--- a/src/ep.h
+++ b/src/ep.h
@@ -946,7 +946,7 @@ protected:
      * @param seqno sequence number of the mutation
      * @param generateBySeqno request that the seqno is generated by this call
      */
-    void tapQueueDirty(RCPtr<VBucket> &vb,
+    void tapQueueDirty(VBucket& vb,
                        StoredValue* v,
                        LockHolder& plh,
                        uint64_t *seqno,
index 44d2d1c..1d06536 100644 (file)
@@ -88,7 +88,7 @@ protected:
 
         queued_item qi{new Item(key, vbucket->getId(), queue_op::set,
                                 /*revSeq*/0, /*bySeq*/0)};
-        return manager->queueDirty(vbucket, qi,
+        return manager->queueDirty(*vbucket, qi,
                                    GenerateBySeqno::Yes, GenerateCas::Yes);
     }
 
@@ -196,7 +196,9 @@ static void launch_set_thread(void *arg) {
         key << "key-" << i;
         queued_item qi(new Item(key.str(), args->vbucket->getId(),
                                 queue_op::set, 0, 0));
-        args->checkpoint_manager->queueDirty(args->vbucket, qi, GenerateBySeqno::Yes, GenerateCas::Yes);
+        args->checkpoint_manager->queueDirty(*args->vbucket, qi,
+                                             GenerateBySeqno::Yes,
+                                             GenerateCas::Yes);
     }
 }
 }
@@ -277,7 +279,8 @@ TEST_F(CheckpointTest, basic_chk_test) {
     // Push the flush command into the queue so that all other threads can be terminated.
     std::string key("flush");
     queued_item qi(new Item(key, vbucket->getId(), queue_op::flush, 0xffff, 0));
-    checkpoint_manager->queueDirty(vbucket, qi, GenerateBySeqno::Yes, GenerateCas::Yes);
+    checkpoint_manager->queueDirty(*vbucket, qi, GenerateBySeqno::Yes,
+                                   GenerateCas::Yes);
 
     rc = cb_join_thread(persistence_thread);
     EXPECT_EQ(0, rc);
@@ -374,7 +377,8 @@ TEST_F(CheckpointTest, OneOpenCkpt) {
 
     // No set_ops in queue, expect queueDirty to return true (increase
     // persistence queue size).
-    EXPECT_TRUE(manager->queueDirty(vbucket, qi, GenerateBySeqno::Yes, GenerateCas::Yes));
+    EXPECT_TRUE(manager->queueDirty(*vbucket, qi, GenerateBySeqno::Yes,
+                                    GenerateCas::Yes));
     EXPECT_EQ(1, manager->getNumCheckpoints());  // Single open checkpoint.
     EXPECT_EQ(2, manager->getNumOpenChkItems()); // 1x op_checkpoint_start, 1x op_set
     EXPECT_EQ(1001, qi->getBySeqno());
@@ -384,7 +388,8 @@ TEST_F(CheckpointTest, OneOpenCkpt) {
     // Adding the same key again shouldn't increase the size.
     queued_item qi2(new Item("key1", vbucket->getId(), queue_op::set,
                             /*revSeq*/21, /*bySeq*/0));
-    EXPECT_FALSE(manager->queueDirty(vbucket, qi2, GenerateBySeqno::Yes, GenerateCas::Yes));
+    EXPECT_FALSE(manager->queueDirty(*vbucket, qi2, GenerateBySeqno::Yes,
+                                     GenerateCas::Yes));
     EXPECT_EQ(1, manager->getNumCheckpoints());
     EXPECT_EQ(2, manager->getNumOpenChkItems());
     EXPECT_EQ(1002, qi2->getBySeqno());
@@ -394,7 +399,8 @@ TEST_F(CheckpointTest, OneOpenCkpt) {
     // Adding a different key should increase size.
     queued_item qi3(new Item("key2", vbucket->getId(), queue_op::set,
                             /*revSeq*/0, /*bySeq*/0));
-    EXPECT_TRUE(manager->queueDirty(vbucket, qi3, GenerateBySeqno::Yes, GenerateCas::Yes));
+    EXPECT_TRUE(manager->queueDirty(*vbucket, qi3, GenerateBySeqno::Yes,
+                                    GenerateCas::Yes));
     EXPECT_EQ(1, manager->getNumCheckpoints());
     EXPECT_EQ(3, manager->getNumOpenChkItems());
     EXPECT_EQ(1003, qi3->getBySeqno());
@@ -923,7 +929,7 @@ TEST_F(CheckpointTest, SeqnoAndHLCOrdering) {
                 queued_item qi(new Item(key + std::to_string(item),
                                         vbucket->getId(), queue_op::set,
                                         /*revSeq*/0, /*bySeq*/0));
-                EXPECT_TRUE(manager->queueDirty(vbucket,
+                EXPECT_TRUE(manager->queueDirty(*vbucket,
                                                 qi,
                                                 GenerateBySeqno::Yes,
                                                 GenerateCas::Yes));