MB-24094: Add Item to SequenceList on TTL update 32/78032/6
authorPremkumar Thangamani <premkumar.thangamani@couchbase.com>
Thu, 11 May 2017 17:58:48 +0000 (10:58 -0700)
committerDave Rigby <daver@couchbase.com>
Wed, 17 May 2017 09:19:28 +0000 (09:19 +0000)
In the GAT Path, when the item is expired, we update the expiry
time. In the case of ephemeral buckets, that item should be updated on
the sequence list.

Change-Id: I2b83456e53cb2d2e4d762d939a716c39c9a725bd
Reviewed-on: http://review.couchbase.org/78032
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
src/ep_vb.cc
src/ep_vb.h
src/ephemeral_vb.cc
src/ephemeral_vb.h
src/vbucket.cc
src/vbucket.h
tests/module_tests/ephemeral_vb_test.cc
tests/module_tests/vbucket_test.cc
tests/module_tests/vbucket_test.h

index 6b1e058..fdda014 100644 (file)
@@ -444,9 +444,14 @@ std::tuple<StoredValue*, MutationStatus, VBNotifyCtx>
 EPVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
                              StoredValue& v,
                              const Item& itm,
-                             const VBQueueItemCtx* queueItmCtx) {
-    MutationStatus status =
-            ht.unlocked_updateStoredValue(hbl.getHTLock(), v, itm);
+                             const VBQueueItemCtx* queueItmCtx,
+                             bool justTouch) {
+    MutationStatus status;
+    if (justTouch) {
+        status = MutationStatus::WasDirty;
+    } else {
+        status = ht.unlocked_updateStoredValue(hbl.getHTLock(), v, itm);
+    }
 
     if (queueItmCtx) {
         return std::make_tuple(&v, status, queueDirty(v, *queueItmCtx));
index 1c0dcb9..449da89 100644 (file)
@@ -145,7 +145,8 @@ private:
             const HashTable::HashBucketLock& hbl,
             StoredValue& v,
             const Item& itm,
-            const VBQueueItemCtx* queueItmCtx) override;
+            const VBQueueItemCtx* queueItmCtx,
+            bool justTouch = false) override;
 
     std::pair<StoredValue*, VBNotifyCtx> addNewStoredValue(
             const HashTable::HashBucketLock& hbl,
@@ -209,4 +210,4 @@ private:
     std::atomic<uint64_t> deferredDeletionFileRevision;
 
     friend class EPVBucketTest;
-};
\ No newline at end of file
+};
index a1b8fc3..5defa9d 100644 (file)
@@ -293,7 +293,8 @@ std::tuple<StoredValue*, MutationStatus, VBNotifyCtx>
 EphemeralVBucket::updateStoredValue(const HashTable::HashBucketLock& hbl,
                                     StoredValue& v,
                                     const Item& itm,
-                                    const VBQueueItemCtx* queueItmCtx) {
+                                    const VBQueueItemCtx* queueItmCtx,
+                                    bool justTouch) {
     std::lock_guard<std::mutex> lh(sequenceLock);
 
     const bool oldValueDeleted = v.isDeleted();
index 22cd85a..8fdb1f3 100644 (file)
@@ -164,7 +164,8 @@ private:
             const HashTable::HashBucketLock& hbl,
             StoredValue& v,
             const Item& itm,
-            const VBQueueItemCtx* queueItmCtx) override;
+            const VBQueueItemCtx* queueItmCtx,
+            bool justTouch = false) override;
 
     std::pair<StoredValue*, VBNotifyCtx> addNewStoredValue(
             const HashTable::HashBucketLock& hbl,
index 93ab6d8..5b47f62 100644 (file)
@@ -1414,46 +1414,48 @@ ENGINE_ERROR_CODE VBucket::add(Item& itm,
     return ENGINE_SUCCESS;
 }
 
-GetValue VBucket::getAndUpdateTtl(const DocKey& key,
-                                  const void* cookie,
-                                  EventuallyPersistentEngine& engine,
-                                  int bgFetchDelay,
-                                  time_t exptime) {
-    auto hbl = ht.getLockedBucket(key);
-    StoredValue* v = fetchValidValue(hbl,
-                                     key,
-                                     WantsDeleted::Yes,
-                                     TrackReference::Yes,
-                                     QueueExpired::Yes);
-
+std::pair<MutationStatus, GetValue> VBucket::processGetAndUpdateTtl(
+        HashTable::HashBucketLock& hbl,
+        const DocKey& key,
+        StoredValue* v,
+        time_t exptime) {
     if (v) {
         if (v->isDeleted() || v->isTempDeletedItem() ||
             v->isTempNonExistentItem()) {
-            return {};
+            return {MutationStatus::NotFound, {}};
         }
 
         if (!v->isResident()) {
-            bgFetch(key, cookie, engine, bgFetchDelay);
-            return GetValue(nullptr, ENGINE_EWOULDBLOCK, v->getBySeqno());
+            return {MutationStatus::NeedBgFetch, {}};
         }
+
         if (v->isLocked(ep_current_time())) {
-            return GetValue(nullptr, ENGINE_KEY_EEXISTS, 0);
+            return {MutationStatus::IsLocked,
+                    GetValue(nullptr, ENGINE_KEY_EEXISTS, 0)};
         }
 
         const bool exptime_mutated = exptime != v->getExptime();
+        auto bySeqNo = v->getBySeqno();
         if (exptime_mutated) {
             v->markDirty();
             v->setExptime(exptime);
             v->setRevSeqno(v->getRevSeqno() + 1);
         }
 
-        GetValue rv(
-                v->toItem(v->isLocked(ep_current_time()), getId()).release(),
-                ENGINE_SUCCESS,
-                v->getBySeqno());
+        Item* item =
+                v->toItem(v->isLocked(ep_current_time()), getId()).release();
+
+        GetValue rv(item, ENGINE_SUCCESS, bySeqNo);
 
         if (exptime_mutated) {
-            VBNotifyCtx notifyCtx = queueDirty(*v);
+            VBQueueItemCtx qItemCtx(GenerateBySeqno::Yes,
+                                    GenerateCas::Yes,
+                                    TrackCasDrift::No,
+                                    false,
+                                    nullptr);
+            VBNotifyCtx notifyCtx;
+            std::tie(v, std::ignore, notifyCtx) =
+                    updateStoredValue(hbl, *v, *item, &qItemCtx, true);
             rv.getValue()->setCas(v->getCas());
             // we unlock ht lock here because we want to avoid potential lock
             // inversions arising from notifyNewSeqno() call
@@ -1461,24 +1463,51 @@ GetValue VBucket::getAndUpdateTtl(const DocKey& key,
             notifyNewSeqno(notifyCtx);
         }
 
-        return rv;
+        return {MutationStatus::WasClean, rv};
     } else {
         if (eviction == VALUE_ONLY) {
-            return {};
+            return {MutationStatus::NotFound, {}};
         } else {
             if (maybeKeyExistsInFilter(key)) {
-                ENGINE_ERROR_CODE ec = addTempItemAndBGFetch(
-                        hbl, key, cookie, engine, bgFetchDelay, false);
-                return GetValue(NULL, ec, -1, true);
+                return {MutationStatus::NeedBgFetch, {}};
             } else {
                 // As bloomfilter predicted that item surely doesn't exist
                 // on disk, return ENOENT for getAndUpdateTtl().
-                return {};
+                return {MutationStatus::NotFound, {}};
             }
         }
     }
 }
 
+GetValue VBucket::getAndUpdateTtl(const DocKey& key,
+                                  const void* cookie,
+                                  EventuallyPersistentEngine& engine,
+                                  int bgFetchDelay,
+                                  time_t exptime) {
+    auto hbl = ht.getLockedBucket(key);
+    StoredValue* v = fetchValidValue(hbl,
+                                     key,
+                                     WantsDeleted::Yes,
+                                     TrackReference::Yes,
+                                     QueueExpired::Yes);
+    GetValue gv;
+    MutationStatus status;
+    std::tie(status, gv) = processGetAndUpdateTtl(hbl, key, v, exptime);
+
+    if (status == MutationStatus::NeedBgFetch) {
+        if (v) {
+            bgFetch(key, cookie, engine, bgFetchDelay);
+            return GetValue(nullptr, ENGINE_EWOULDBLOCK, v->getBySeqno());
+        } else {
+            ENGINE_ERROR_CODE ec = addTempItemAndBGFetch(
+                    hbl, key, cookie, engine, bgFetchDelay, false);
+            return GetValue(NULL, ec, -1, true);
+        }
+    }
+
+    return gv;
+}
+
 MutationStatus VBucket::insertFromWarmup(Item& itm,
                                          bool eject,
                                          bool keyMetaDataOnly) {
index 84aa25a..73946e9 100644 (file)
@@ -1223,6 +1223,15 @@ public:
 
 protected:
     /**
+     * This function checks for the various states of the value & depending on
+     * which the calling function can issue a bgfetch as needed.
+     */
+    std::pair<MutationStatus, GetValue> processGetAndUpdateTtl(
+            HashTable::HashBucketLock& hbl,
+            const DocKey& key,
+            StoredValue* v,
+            time_t exptime);
+    /**
      * This function checks cas, expiry and other partition (vbucket) related
      * rules before setting an item into other in-memory structure like HT,
      * and checkpoint mgr. This function assumes that HT bucket lock is grabbed.
@@ -1468,7 +1477,8 @@ private:
      * @param itm Item to be updated.
      * @param queueItmCtx holds info needed to queue an item in chkpt or vb
      *                    backfill queue; NULL if item need not be queued
-     *
+     * @param justTouch   To note that this object is an existing item with
+     *                    the same value but with few flags changed.
      * @return pointer to the updated StoredValue. It can be same as that of
      *         v or different value if a new StoredValue is created for the
      *         update.
@@ -1479,7 +1489,8 @@ private:
     updateStoredValue(const HashTable::HashBucketLock& hbl,
                       StoredValue& v,
                       const Item& itm,
-                      const VBQueueItemCtx* queueItmCtx) = 0;
+                      const VBQueueItemCtx* queueItmCtx,
+                      bool justTouch = false) = 0;
 
     /**
      * Adds a new StoredValue in in-memory data structures like HT.
index 30cc445..46626ce 100644 (file)
@@ -222,6 +222,62 @@ TEST_F(EphemeralVBucketTest, UpdateDuringBackfill) {
               mockEpheVB->public_getNumListItems());
 }
 
+TEST_F(EphemeralVBucketTest, GetAndUpdateTtl) {
+    const int numItems = 2;
+
+    /* Add 2 keys */
+    auto keys = generateKeys(numItems);
+    setMany(keys, MutationStatus::WasClean);
+
+    ASSERT_EQ(numItems, vbucket->getNumItems());
+    EXPECT_EQ(numItems, vbucket->getHighSeqno());
+    EXPECT_EQ(0, mockEpheVB->public_getNumStaleItems());
+
+    /* --- basic test --- */
+    /* set the ttl of one item */
+    GetValue gv1 = public_getAndUpdateTtl(keys[0], 100);
+
+    /* New seqno should have been used */
+    EXPECT_EQ(numItems + 1, vbucket->getHighSeqno());
+
+    /* No.of items in the bucket should NOT change */
+    EXPECT_EQ(numItems, vbucket->getNumItems());
+
+    /* No.of items in the list should NOT change */
+    EXPECT_EQ(numItems, mockEpheVB->public_getNumListItems());
+
+    /* There should be NO stale items */
+    EXPECT_EQ(0, mockEpheVB->public_getNumStaleItems());
+
+    /* --- Repeat the above test with a similated ReadRange --- */
+    mockEpheVB->registerFakeReadRange(1, numItems);
+    GetValue gv2 = public_getAndUpdateTtl(keys[1], 101);
+
+    /* New seqno should have been used */
+    EXPECT_EQ(numItems + 2, vbucket->getHighSeqno());
+
+    /* No.of items in the bucket should remain the same */
+    EXPECT_EQ(numItems, vbucket->getNumItems());
+
+    /* No.of items in the sequence list should inc by 1 */
+    EXPECT_EQ(numItems + 1, mockEpheVB->public_getNumListItems());
+
+    /* There should be 1 stale item */
+    EXPECT_EQ(1, mockEpheVB->public_getNumStaleItems());
+
+    auto seqNoVec = mockEpheVB->getLL()->getAllSeqnoForVerification();
+    seqno_t prevSeqNo = 0;
+
+    for (const auto& seqNo : seqNoVec) {
+        EXPECT_GT(seqNo, prevSeqNo);
+        prevSeqNo = seqNo;
+    }
+
+    // explicit delete to keep valgrind happy
+    delete (Item*)gv1.getValue();
+    delete (Item*)gv2.getValue();
+}
+
 TEST_F(EphemeralVBucketTest, SoftDeleteDuringBackfill) {
     /* Add 5 items and then soft delete all of them */
     const int numItems = 5;
index ea52bef..b54fb03 100644 (file)
@@ -188,6 +188,16 @@ bool VBucketTest::public_deleteStoredValue(const DocKey& key) {
     return vbucket->deleteStoredValue(hbl_sv.first, *hbl_sv.second);
 }
 
+GetValue VBucketTest::public_getAndUpdateTtl(const DocKey& key,
+                                             time_t exptime) {
+    auto hbl = lockAndFind(key);
+    GetValue gv;
+    MutationStatus status;
+    std::tie(status, gv) = vbucket->processGetAndUpdateTtl(
+            hbl.first, key, hbl.second, exptime);
+    return gv;
+}
+
 size_t EPVBucketTest::public_queueBGFetchItem(
         const DocKey& key,
         std::unique_ptr<VBucketBGFetchItem> fetchItem,
index 9b2d182..9f697a1 100644 (file)
@@ -86,6 +86,8 @@ protected:
 
     bool public_deleteStoredValue(const DocKey& key);
 
+    GetValue public_getAndUpdateTtl(const DocKey& key, time_t exptime);
+
     std::unique_ptr<VBucket> vbucket;
     EPStats global_stats;
     CheckpointConfig checkpoint_config;