MB-38444: Remove the SequenceList::rangeRead API

paolococchi · daverigby · commit 802eba74f77a · 2021-01-26T11:46:35.000Z
Not used in production. Change-Id: Id7343040d72f77ea6b75711aff0830c99f9eb0ae Reviewed-on: http://review.couchbase.org/c/kv_engine/+/144255 Well-Formed: Build Bot <build@couchbase.com> Reviewed-by: Dave Rigby <daver@couchbase.com> Tested-by: Build Bot <build@couchbase.com>
diff --git a/engines/ep/src/ephemeral_vb.cc b/engines/ep/src/ephemeral_vb.cc
@@ -316,15 +316,11 @@ std::unique_ptr<DCPBackfillIface> EphemeralVBucket::createDCPBackfill(
             evb, stream, startSeqno, endSeqno);
 }
 
-std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
-EphemeralVBucket::inMemoryBackfill(uint64_t start, uint64_t end) {
-    return seqList->rangeRead(start, end);
-}
-
 boost::optional<SequenceList::RangeIterator>
 EphemeralVBucket::makeRangeIterator(bool isBackfill) {
     return seqList->makeRangeIterator(isBackfill);
 }
+
 bool EphemeralVBucket::isKeyLogicallyDeleted(const DocKey& key,
                                              int64_t bySeqno) {
     auto cid = key.getCollectionID();
diff --git a/engines/ep/src/ephemeral_vb.h b/engines/ep/src/ephemeral_vb.h
@@ -128,25 +128,6 @@ class EphemeralVBucket : public VBucket {
             uint64_t startSeqno,
             uint64_t endSeqno) override;
 
-    /**
-     * Reads backfill items from in memory ordered data structure.
-     *
-     * Because the backfill may have to be extended to ensure consistency (e.g.,
-     * an item in the range has been updated and the new version is
-     * outside of the original range would result in a missing item), the
-     * end of the range may be at a higher seqno than was requested; this new
-     * end value is returned.
-     *
-     * @param startSeqno requested start sequence number of the backfill
-     * @param endSeqno requested end sequence number of the backfill
-     *
-     * @return ENGINE_SUCCESS, items in the snapshot, adjusted endSeqno
-     *         ENGINE_ENOMEM on no memory to copy items
-     *         ENGINE_ERANGE on incorrect start and end
-     */
-    std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
-    inMemoryBackfill(uint64_t start, uint64_t end);
-
     /**
      * Creates a range iterator for the underlying SequenceList 'optionally'.
      * Under scenarios like where we want to limit the number of range iterators
diff --git a/engines/ep/src/linked_list.cc b/engines/ep/src/linked_list.cc
@@ -89,107 +89,6 @@ SequenceList::UpdateStatus BasicLinkedList::updateListElem(
     return UpdateStatus::Success;
 }
 
-std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
-BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
-    if ((start > end) || (start <= 0)) {
-        EP_LOG_WARN(
-                "BasicLinkedList::rangeRead(): ({}) ERANGE: start {} > end {}",
-                vbid,
-                start,
-                end);
-        return std::make_tuple(ENGINE_ERANGE, std::vector<UniqueItemPtr>(), 0);
-    }
-
-    RangeGuard range;
-
-    {
-        std::lock_guard<std::mutex> listWriteLg(getListWriteLock());
-
-        if (start > highSeqno) {
-            EP_LOG_WARN(
-                    "BasicLinkedList::rangeRead(): "
-                    "({}) ERANGE: start {} > highSeqno {}",
-                    vbid,
-                    start,
-                    static_cast<seqno_t>(highSeqno));
-            /* If the request is for an invalid range, return before iterating
-               through the list */
-            return std::make_tuple(
-                    ENGINE_ERANGE, std::vector<UniqueItemPtr>(), 0);
-        }
-
-        /* Mark the initial read range */
-        end = std::min(end, static_cast<seqno_t>(highSeqno));
-        end = std::max(end, static_cast<seqno_t>(highestDedupedSeqno));
-
-        // the range lock will be released when the RangeGuard is destroyed.
-        range = tryLockSeqnoRangeShared(1, end);
-        if (!range) {
-            return std::make_tuple(
-                    ENGINE_TMPFAIL, std::vector<UniqueItemPtr>(), 0);
-        }
-    }
-
-    /* Read items in the range */
-    std::vector<UniqueItemPtr> items;
-
-    for (const auto& osv : seqList) {
-        int64_t currSeqno(osv.getBySeqno());
-
-        if (currSeqno > end || currSeqno < 0) {
-            /* We have read all the items in the requested range, or the osv
-             * does not yet have a valid seqno; either way we are done */
-            break;
-        }
-
-        if (currSeqno < start) {
-            /* skip this item */
-            continue;
-        }
-
-        if (currSeqno > 1) {
-            // strictly monotonically update the range lock
-            range.updateRangeStart(currSeqno);
-            /* [EPHE TODO] MB-37710: Will updating the range start every time
-             * have a negative performance impact? */
-        }
-
-        /* Check if this OSV has been made stale and has been superseded by a
-         * newer version. If it has, and the replacement is /also/ in the range
-         * we are reading, we should skip this item to avoid duplicates */
-        StoredValue* replacement;
-        {
-            std::lock_guard<std::mutex> writeGuard(getListWriteLock());
-            replacement = osv.getReplacementIfStale(writeGuard);
-        }
-
-        if (replacement &&
-            replacement->toOrderedStoredValue()->getBySeqno() <= end) {
-            continue;
-        }
-
-        try {
-            items.push_back(UniqueItemPtr(osv.toItem(vbid)));
-        } catch (const std::bad_alloc&) {
-            /* [EPHE TODO]: Do we handle backfill in a better way ?
-                            Perhaps do backfilling partially (that is
-                            backfill ==> stream; backfill ==> stream ..so on )?
-             */
-            EP_LOG_WARN(
-                    "BasicLinkedList::rangeRead(): "
-                    "({}) ENOMEM while trying to copy "
-                    "item with seqno {} before streaming it",
-                    vbid,
-                    currSeqno);
-            return std::make_tuple(
-                    ENGINE_ENOMEM, std::vector<UniqueItemPtr>(), 0);
-        }
-    }
-
-    /* Return all the range read items */
-    return std::make_tuple(ENGINE_SUCCESS, std::move(items), end);
-}
-
 void BasicLinkedList::updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
                                       const OrderedStoredValue& v) {
     if (v.getBySeqno() < 1) {
diff --git a/engines/ep/src/linked_list.h b/engines/ep/src/linked_list.h
@@ -96,9 +96,6 @@ class BasicLinkedList : public SequenceList {
             std::lock_guard<std::mutex>& writeLock,
             OrderedStoredValue& v) override;
 
-    std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
-    rangeRead(seqno_t start, seqno_t end) override;
-
     void updateHighSeqno(std::lock_guard<std::mutex>& listWriteLg,
                          const OrderedStoredValue& v) override;
 
diff --git a/engines/ep/src/seqlist.h b/engines/ep/src/seqlist.h
@@ -264,27 +264,6 @@ class SequenceList {
             std::lock_guard<std::mutex>& writeLock,
             OrderedStoredValue& v) = 0;
 
-    /**
-     * Provides point-in-time snapshots which can be used for incremental
-     * replication.
-     *
-     * Copies the StoredValues as a vector of ref counterd items starting from
-     * 'start + 1' seqno into 'items' as a snapshot.
-     *
-     * Since we use monotonically increasing point-in-time snapshots we cannot
-     * guarantee that the snapshot ends at the requested end seqno. Due to
-     * dedups we may have to send till a higher seqno in the snapshot.
-     *
-     * @param start requested start seqno
-     * @param end requested end seqno
-     *
-     * @return ENGINE_SUCCESS, items in the snapshot and adjusted endSeqNo
-     *         ENGINE_ENOMEM on no memory to copy items
-     *         ENGINE_ERANGE on incorrect start and end
-     */
-    virtual std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
-    rangeRead(seqno_t start, seqno_t end) = 0;
-
     /**
      * Updates the highSeqno in the list. Since seqno is generated and managed
      * outside the list, the module managing it must update this after the seqno
diff --git a/engines/ep/tests/module_tests/basic_ll_test.cc b/engines/ep/tests/module_tests/basic_ll_test.cc
@@ -270,98 +270,6 @@ TEST_F(BasicLinkedListTest, SetItems) {
     EXPECT_EQ(expectedSeqno, basicLL->getAllSeqnoForVerification());
 }
 
-TEST_F(BasicLinkedListTest, TestRangeRead) {
-    const int numItems = 3;
-
-    /* Add 3 new items */
-    addNewItemsToList(1, std::string("key"), numItems);
-
-    /* Now do a range read */
-    ENGINE_ERROR_CODE status;
-    std::vector<UniqueItemPtr> items;
-    seqno_t endSeqno;
-    std::tie(status, items, endSeqno) = basicLL->rangeRead(1, numItems);
-
-    EXPECT_EQ(ENGINE_SUCCESS, status);
-    EXPECT_EQ(numItems, items.size());
-    EXPECT_EQ(numItems, items.back()->getBySeqno());
-    EXPECT_EQ(numItems, endSeqno);
-}
-
-TEST_F(BasicLinkedListTest, TestRangeReadTillInf) {
-    const int numItems = 3;
-
-    /* Add 3 new items */
-    addNewItemsToList(1, std::string("key"), numItems);
-
-    /* Now do a range read */
-    ENGINE_ERROR_CODE status;
-    std::vector<UniqueItemPtr> items;
-    seqno_t endSeqno;
-    std::tie(status, items, endSeqno) =
-            basicLL->rangeRead(1, std::numeric_limits<seqno_t>::max());
-
-    EXPECT_EQ(ENGINE_SUCCESS, status);
-    EXPECT_EQ(numItems, items.size());
-    EXPECT_EQ(numItems, items.back()->getBySeqno());
-    EXPECT_EQ(numItems, endSeqno);
-}
-
-TEST_F(BasicLinkedListTest, TestRangeReadFromMid) {
-    const int numItems = 3;
-
-    /* Add 3 new items */
-    addNewItemsToList(1, std::string("key"), numItems);
-
-    /* Now do a range read */
-    ENGINE_ERROR_CODE status;
-    std::vector<UniqueItemPtr> items;
-    seqno_t endSeqno;
-    std::tie(status, items, endSeqno) = basicLL->rangeRead(2, numItems);
-
-    EXPECT_EQ(ENGINE_SUCCESS, status);
-    EXPECT_EQ(numItems - 1, items.size());
-    EXPECT_EQ(numItems, items.back()->getBySeqno());
-    EXPECT_EQ(numItems, endSeqno);
-}
-
-TEST_F(BasicLinkedListTest, TestRangeReadStopBeforeEnd) {
-    const int numItems = 3;
-
-    /* Add 3 new items */
-    addNewItemsToList(1, std::string("key"), numItems);
-
-    /* Now request for a range read of just 2 items */
-    ENGINE_ERROR_CODE status;
-    std::vector<UniqueItemPtr> items;
-    seqno_t endSeqno;
-    std::tie(status, items, endSeqno) = basicLL->rangeRead(1, numItems - 1);
-
-    EXPECT_EQ(ENGINE_SUCCESS, status);
-    EXPECT_EQ(numItems - 1, items.size());
-    EXPECT_EQ(numItems - 1, items.back()->getBySeqno());
-    EXPECT_EQ(numItems - 1, endSeqno);
-}
-
-TEST_F(BasicLinkedListTest, TestRangeReadNegatives) {
-    const int numItems = 3;
-
-    /* Add 3 new items */
-    addNewItemsToList(1, std::string("key"), numItems);
-
-    ENGINE_ERROR_CODE status;
-    std::vector<UniqueItemPtr> items;
-
-    /* Now do a range read with start > end */
-    std::tie(status, items, std::ignore) = basicLL->rangeRead(2, 1);
-    EXPECT_EQ(ENGINE_ERANGE, status);
-
-    /* Now do a range read with start > highSeqno */
-    std::tie(status, items, std::ignore) =
-            basicLL->rangeRead(numItems + 1, numItems + 2);
-    EXPECT_EQ(ENGINE_ERANGE, status);
-}
-
 TEST_F(BasicLinkedListTest, UpdateFirstElem) {
     const int numItems = 3;
     const std::string keyPrefix("key");
@@ -886,28 +794,6 @@ TEST_F(BasicLinkedListTest,
     EXPECT_FALSE(guard2);
 }
 
-TEST_F(BasicLinkedListTest, RangeReadStopsOnInvalidSeqno) {
-    /* MB-24376: rangeRead has to stop if it encounters an OSV with a seqno of
-     * -1; this item is definitely past the end of the rangeRead, and has not
-     * yet had its seqno updated in queueDirty */
-    const int numItems = 2;
-    const std::string keyPrefix("key");
-
-    /* Add 2 new items */
-    addNewItemsToList(1, keyPrefix, numItems);
-
-    /* Add a key that does not yet have a vaild seqno (say -1) */
-    addItemWithoutSeqno("key3");
-
-    EXPECT_EQ(-1, basicLL->getSeqList().back().getBySeqno());
-
-    auto res = basicLL->rangeRead(1, std::numeric_limits<seqno_t>::max());
-
-    EXPECT_EQ(ENGINE_SUCCESS, std::get<0>(res));
-    EXPECT_EQ(numItems, std::get<1>(res).size());
-    EXPECT_EQ(numItems, std::get<2>(res));
-}
-
 /* 'EphemeralVBucket' (class that has the list) never calls the purge of last
    element, but the list must support generic purge (that is purge until any
    element). */
diff --git a/engines/ep/tests/module_tests/ephemeral_vb_test.cc b/engines/ep/tests/module_tests/ephemeral_vb_test.cc
