MB-20623: MutationLog: Remove unused Delete and Uncommitted functionality 41/68041/7
authorDave Rigby <daver@couchbase.com>
Mon, 22 Aug 2016 13:10:17 +0000 (14:10 +0100)
committerDave Rigby <daver@couchbase.com>
Fri, 30 Sep 2016 15:06:52 +0000 (15:06 +0000)
The MutationLog (when actually used for Mutations) could record
Deletes (in addition to 'adds') of keys, and return a list of
'uncommitted' keys.  However this functionality has long been unused
(since March 2013, http://review.couchbase.org/26943).

Remove this unused code to simplify the way the MutationLog class is
now used - for the access log.

Change-Id: I5642553708c7d5cf320cec5524ae49643a414192
Reviewed-on: http://review.couchbase.org/68041
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
src/mutation_log.cc
src/mutation_log.h
tests/module_tests/mutation_log_test.cc

index d96cde2..a126426 100644 (file)
@@ -305,24 +305,6 @@ void MutationLog::newItem(uint16_t vbucket, const std::string &key,
     }
 }
 
-void MutationLog::delItem(uint16_t vbucket, const std::string &key) {
-    if (isEnabled()) {
-        MutationLogEntry *mle = MutationLogEntry::newEntry(entryBuffer.get(),
-                                                           0, ML_DEL, vbucket,
-                                                           key);
-        writeEntry(mle);
-    }
-}
-
-void MutationLog::deleteAll(uint16_t vbucket) {
-    if (isEnabled()) {
-        MutationLogEntry *mle = MutationLogEntry::newEntry(entryBuffer.get(),
-                                                           0, ML_DEL_ALL,
-                                                           vbucket, "");
-        writeEntry(mle);
-    }
-}
-
 void MutationLog::sync() {
     if (!isOpen()) {
         throw std::logic_error("MutationLog::sync: Not valid on a closed log");
@@ -739,12 +721,6 @@ static const char* logType(uint8_t t) {
     case ML_NEW:
         return "new";
         break;
-    case ML_DEL:
-        return "del";
-        break;
-    case ML_DEL_ALL:
-        return "delall";
-        break;
     case ML_COMMIT1:
         return "commit1";
         break;
@@ -899,8 +875,6 @@ bool MutationLogHarvester::load() {
         clean = false;
 
         switch (le->type()) {
-        case ML_DEL:
-            // FALLTHROUGH
         case ML_NEW:
             if (vbid_set.find(le->vbucket()) != vbid_set.end()) {
                 loading[le->vbucket()][le->key()] =
@@ -909,10 +883,6 @@ bool MutationLogHarvester::load() {
             break;
         case ML_COMMIT2: {
             clean = true;
-            for (const uint16_t vb :shouldClear) {
-                committed[vb].clear();
-            }
-            shouldClear.clear();
 
             for (const uint16_t vb : vbid_set) {
                 for (auto& copyit2 : loading[vb]) {
@@ -923,9 +893,6 @@ bool MutationLogHarvester::load() {
                     case ML_NEW:
                         committed[vb][copyit2.first] = t.first;
                         break;
-                    case ML_DEL:
-                        committed[vb].erase(copyit2.first);
-                        break;
                     default:
                         abort();
                     }
@@ -937,12 +904,6 @@ bool MutationLogHarvester::load() {
         case ML_COMMIT1:
             // nothing in particular
             break;
-        case ML_DEL_ALL:
-            if (vbid_set.find(le->vbucket()) != vbid_set.end()) {
-                loading[le->vbucket()].clear();
-                shouldClear.insert(le->vbucket());
-            }
-            break;
         default:
             abort();
         }
@@ -989,25 +950,6 @@ void MutationLogHarvester::apply(void *arg, mlCallbackWithQueue mlc) {
     }
 }
 
-void MutationLogHarvester::getUncommitted(
-                             std::vector<mutation_log_uncommitted_t> &uitems) {
-
-    for (const uint16_t vb : vbid_set) {
-        mutation_log_uncommitted_t leftover;
-        leftover.vbucket = vb;
-
-        for (const auto& copyit2 : loading[vb]) {
-
-            const mutation_log_event_t& t = copyit2.second;
-            leftover.key = copyit2.first;
-            leftover.rowid = t.first;
-            leftover.type = static_cast<mutation_log_type_t>(t.second);
-
-            uitems.push_back(leftover);
-        }
-    }
-}
-
 size_t MutationLogHarvester::total() {
     size_t rv(0);
     for (int i = 0; i < MUTATION_LOG_TYPES; ++i) {
index 3da83b2..6c36ee3 100644 (file)
 
 #pragma once
 
+/**
+ * 'Mutation' Log
+ *
+ * The MutationLog is used to maintain a log of mutations which have occurred
+ * in one or more vbuckets. It only records the additions or removals of keys,
+ * and then only the key of the item (no value).
+ *
+ * The original intent of this class was to record a log in parallel with the
+ * normal couchstore snapshots, see docs/klog.org, however this has not been
+ * used since MB-7590 (March 2013).
+ *
+ * The current use of MutationLog is for the access.log. This is a slightly
+ * different use-case - periodically (default daily) the AccessScanner walks
+ * each vBucket's HashTable and records the set of keys currently resident.
+ * This doesn't make use of the MutationLog's commit functionality - its simply
+ * a list of keys which were resident. When we later come to read the Access log
+ * during warmup there's no guarantee that the keys listed still exist - the
+ * contents of the Access log is essentially just a hint / suggestion.
+ *
+ */
+
 #include "config.h"
 
 #include <array>
@@ -172,7 +193,11 @@ private:
 };
 
 enum mutation_log_type_t {
-    ML_NEW, ML_DEL, ML_DEL_ALL, ML_COMMIT1, ML_COMMIT2
+    ML_NEW = 0,
+    /* removed: ML_DEL = 1 */
+    /* removed: ML_DEL_ALL = 2 */
+    ML_COMMIT1 = 3,
+    ML_COMMIT2 = 4
 };
 
 #define MUTATION_LOG_TYPES 5
@@ -322,10 +347,6 @@ public:
 
     void newItem(uint16_t vbucket, const std::string &key, uint64_t rowid);
 
-    void delItem(uint16_t vbucket, const std::string &key);
-
-    void deleteAll(uint16_t vbucket);
-
     void commit1();
 
     void commit2();
@@ -605,11 +626,6 @@ public:
         return itemsSeen;
     }
 
-    /**
-     * Get the list of uncommitted keys and stuff from the log.
-     */
-    void getUncommitted(std::vector<mutation_log_uncommitted_t> &uitems);
-
 private:
 
     MutationLog &mlog;
index e909fc5..32aa60f 100644 (file)
@@ -101,8 +101,6 @@ TEST_F(MutationLogTest, Unconfigured) {
     ml.open();
     ASSERT_FALSE(ml.isEnabled());
     ml.newItem(3, "somekey", 931);
-    ml.delItem(3, "somekey");
-    ml.deleteAll(3);
     ml.commit1();
     ml.commit2();
     ml.flush();
@@ -197,18 +195,15 @@ TEST_F(MutationLogTest, Logging) {
         MutationLog ml(tmp_log_filename.c_str());
         ml.open();
 
-        ml.newItem(3, "key1", 1);
         ml.newItem(2, "key1", 2);
         ml.commit1();
         ml.commit2();
         ml.newItem(3, "key2", 3);
-        ml.delItem(3, "key1");
         ml.commit1();
         ml.commit2();
         // Remaining:   3:key2, 2:key1
 
-        EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL]);
+        EXPECT_EQ(2, ml.itemsLogged[ML_NEW]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT1]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT2]);
     }
@@ -223,16 +218,14 @@ TEST_F(MutationLogTest, Logging) {
 
         EXPECT_TRUE(h.load());
 
-        EXPECT_EQ(3, h.getItemsSeen()[ML_NEW]);
-        EXPECT_EQ(1, h.getItemsSeen()[ML_DEL]);
+        EXPECT_EQ(2, h.getItemsSeen()[ML_NEW]);
         EXPECT_EQ(2, h.getItemsSeen()[ML_COMMIT1]);
         EXPECT_EQ(2, h.getItemsSeen()[ML_COMMIT2]);
 
         // Check stat copying
         ml.resetCounts(h.getItemsSeen());
 
-        EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL]);
+        EXPECT_EQ(2, ml.itemsLogged[ML_NEW]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT1]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT2]);
 
@@ -250,83 +243,6 @@ TEST_F(MutationLogTest, Logging) {
     }
 }
 
-TEST_F(MutationLogTest, DelAll) {
-    remove(tmp_log_filename.c_str());
-
-    {
-        MutationLog ml(tmp_log_filename.c_str());
-        ml.open();
-
-        ml.newItem(3, "key1", 1);
-        ml.newItem(2, "key1", 2);
-        ml.commit1();
-        ml.commit2();
-        ml.newItem(3, "key2", 3);
-        ml.deleteAll(3);
-        ml.commit1();
-        ml.commit2();
-        // Remaining:   2:key1
-
-        EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(0, ml.itemsLogged[ML_DEL]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL_ALL]);
-        EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT1]);
-        EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT2]);
-    }
-
-    {
-        MutationLog ml(tmp_log_filename.c_str());
-        ml.open();
-        MutationLogHarvester h(ml);
-        h.setVBucket(1);
-        h.setVBucket(2);
-        h.setVBucket(3);
-
-        EXPECT_TRUE(h.load());
-
-        EXPECT_EQ(3, h.getItemsSeen()[ML_NEW]);
-        EXPECT_EQ(1, h.getItemsSeen()[ML_DEL_ALL]);
-        EXPECT_EQ(2, h.getItemsSeen()[ML_COMMIT1]);
-        EXPECT_EQ(2, h.getItemsSeen()[ML_COMMIT2]);
-
-        // Check stat copying
-        ml.resetCounts(h.getItemsSeen());
-
-        EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL_ALL]);
-        EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT1]);
-        EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT2]);
-
-        // See if we got what we expect.
-        std::map<std::string, uint64_t> maps[4];
-        h.apply(&maps, loaderFun);
-
-        EXPECT_EQ(0, maps[0].size());
-        EXPECT_EQ(0, maps[1].size());
-        EXPECT_EQ(1, maps[2].size());
-        EXPECT_EQ(0, maps[3].size());
-
-        EXPECT_NE(maps[2].end(), maps[2].find("key1"));
-    }
-}
-
-static bool leftover_compare(mutation_log_uncommitted_t a,
-                             mutation_log_uncommitted_t b) {
-    if (a.vbucket != b.vbucket) {
-        return a.vbucket < b.vbucket;
-    }
-
-    if (a.key != b.key) {
-        return a.key < b.key;
-    }
-
-    if (a.type != b.type) {
-        return a.type < b.type;
-    }
-
-    return false;
-}
-
 TEST_F(MutationLogTest, LoggingDirty) {
     remove(tmp_log_filename.c_str());
 
@@ -338,14 +254,12 @@ TEST_F(MutationLogTest, LoggingDirty) {
         ml.newItem(2, "key1", 2);
         ml.commit1();
         ml.commit2();
-        // These two will be dropped from the normal loading path
+        // This will be dropped from the normal loading path
         // because there's no commit.
         ml.newItem(3, "key2", 3);
-        ml.delItem(3, "key1");
         // Remaining:   3:key1, 2:key1
 
         EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL]);
         EXPECT_EQ(1, ml.itemsLogged[ML_COMMIT1]);
         EXPECT_EQ(1, ml.itemsLogged[ML_COMMIT2]);
     }
@@ -361,7 +275,6 @@ TEST_F(MutationLogTest, LoggingDirty) {
         EXPECT_FALSE(h.load());
 
         EXPECT_EQ(3, h.getItemsSeen()[ML_NEW]);
-        EXPECT_EQ(1, h.getItemsSeen()[ML_DEL]);
         EXPECT_EQ(1, h.getItemsSeen()[ML_COMMIT1]);
         EXPECT_EQ(1, h.getItemsSeen()[ML_COMMIT2]);
 
@@ -369,7 +282,6 @@ TEST_F(MutationLogTest, LoggingDirty) {
         ml.resetCounts(h.getItemsSeen());
 
         EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL]);
         EXPECT_EQ(1, ml.itemsLogged[ML_COMMIT1]);
         EXPECT_EQ(1, ml.itemsLogged[ML_COMMIT2]);
 
@@ -386,18 +298,6 @@ TEST_F(MutationLogTest, LoggingDirty) {
         EXPECT_NE(maps[3].end(), maps[3].find("key1"));
         EXPECT_EQ(maps[3].end(), maps[3].find("key2"));
 
-        // Check the leftovers
-        std::vector<mutation_log_uncommitted_t> leftovers;
-        h.getUncommitted(leftovers);
-        std::sort(leftovers.begin(), leftovers.end(), leftover_compare);
-        EXPECT_EQ(2, leftovers.size());
-        EXPECT_EQ(3, leftovers[0].vbucket);
-        EXPECT_EQ("key1", leftovers[0].key);
-        EXPECT_EQ(ML_DEL, leftovers[0].type);
-        EXPECT_EQ(3, leftovers[1].vbucket);
-        EXPECT_EQ("key2", leftovers[1].key);
-        EXPECT_EQ(ML_NEW, leftovers[1].type);
-        EXPECT_EQ(3, leftovers[1].rowid);
     }
 }
 
@@ -408,18 +308,15 @@ TEST_F(MutationLogTest, LoggingBadCRC) {
         MutationLog ml(tmp_log_filename.c_str());
         ml.open();
 
-        ml.newItem(3, "key1", 1);
         ml.newItem(2, "key1", 2);
         ml.commit1();
         ml.commit2();
         ml.newItem(3, "key2", 3);
-        ml.delItem(3, "key1");
         ml.commit1();
         ml.commit2();
         // Remaining:   3:key2, 2:key1
 
-        EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL]);
+        EXPECT_EQ(2, ml.itemsLogged[ML_NEW]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT1]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT2]);
     }
@@ -445,7 +342,6 @@ TEST_F(MutationLogTest, LoggingBadCRC) {
         EXPECT_THROW(h.load(), MutationLog::CRCReadException);
 
         EXPECT_EQ(0, h.getItemsSeen()[ML_NEW]);
-        EXPECT_EQ(0, h.getItemsSeen()[ML_DEL]);
         EXPECT_EQ(0, h.getItemsSeen()[ML_COMMIT1]);
         EXPECT_EQ(0, h.getItemsSeen()[ML_COMMIT2]);
 
@@ -453,7 +349,6 @@ TEST_F(MutationLogTest, LoggingBadCRC) {
         ml.resetCounts(h.getItemsSeen());
 
         EXPECT_EQ(0, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(0, ml.itemsLogged[ML_DEL]);
         EXPECT_EQ(0, ml.itemsLogged[ML_COMMIT1]);
         EXPECT_EQ(0, ml.itemsLogged[ML_COMMIT2]);
 
@@ -478,18 +373,15 @@ TEST_F(MutationLogTest, LoggingShortRead) {
         MutationLog ml(tmp_log_filename.c_str());
         ml.open();
 
-        ml.newItem(3, "key1", 1);
         ml.newItem(2, "key1", 2);
         ml.commit1();
         ml.commit2();
         ml.newItem(3, "key2", 3);
-        ml.delItem(3, "key1");
         ml.commit1();
         ml.commit2();
         // Remaining:   3:key2, 2:key1
 
-        EXPECT_EQ(3, ml.itemsLogged[ML_NEW]);
-        EXPECT_EQ(1, ml.itemsLogged[ML_DEL]);
+        EXPECT_EQ(2, ml.itemsLogged[ML_NEW]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT1]);
         EXPECT_EQ(2, ml.itemsLogged[ML_COMMIT2]);
     }