MB-43199: Run compaction with a config representing multiple requests

If multiple compaction requests occurred before the task runs, the
config of the last schedule request is what will be used in the run.

For example a user may issue a manual compaction (via cbepctl) which
requests all deletes are dropped, whilst ns_server requests a normal
compaction. The user's command may return success, yet a compaction
with drop_deletes=true may never of executed. Prior to MB-41782
the user's request would of been honoured because separate compactions
would of been scheduled.

This commit uses a merge function on the CompactionConfig so that
all schedules 'merge' the requested config into the scheduled task's
config. When the CompactionTask runs it now has a config which
respects all requests.

Change-Id: I6e2d373e0c6f36dd946db06c3d6953a7481c1eae
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/143381
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jimwwalker

engines/ep/src/kvstore.cc

@@ -622,9 +622,39 @@ bool IORequest::isDelete() const {
     return item->isDeleted() && !item->isPending();
 }
 
+CompactionConfig::CompactionConfig(CompactionConfig&& other) {
+    *this = std::move(other);
+}
+
+CompactionConfig& CompactionConfig::operator=(CompactionConfig&& other) {
+    if (*this != other) {
+        purge_before_ts = other.purge_before_ts;
+        purge_before_seq = other.purge_before_seq;
+        drop_deletes = other.drop_deletes;
+        retain_erroneous_tombstones = other.retain_erroneous_tombstones;
+        other = CompactionConfig{}; // back to default values
+    }
+    return *this;
+}
+
 bool CompactionConfig::operator==(const CompactionConfig& other) const {
     return purge_before_ts == other.purge_before_ts &&
            purge_before_seq == other.purge_before_seq &&
            drop_deletes == other.drop_deletes &&
            retain_erroneous_tombstones == other.retain_erroneous_tombstones;
 }
+
+// Merge other into this.
+// drop_deletes and retain_erroneous_tombstones are 'sticky' once true
+// they should stay true for all subsequent merges.
+// purge_before_ts and purge_before_seq should be the largest value out of
+// the current and the input
+void CompactionConfig::merge(const CompactionConfig& other) {
+    retain_erroneous_tombstones =
+            retain_erroneous_tombstones || other.retain_erroneous_tombstones;
+    drop_deletes = drop_deletes || other.drop_deletes;
+    purge_before_ts =
+            std::max<uint64_t>(purge_before_ts, other.purge_before_ts);
+    purge_before_seq =
+            std::max<uint64_t>(purge_before_seq, other.purge_before_seq);
+}

engines/ep/src/kvstore.h

@@ -98,14 +98,44 @@ struct CompactionStats {
 };
 
 struct CompactionConfig {
-    uint64_t purge_before_ts = 0;
-    uint64_t purge_before_seq = 0;
-    uint8_t drop_deletes = 0;
-    bool retain_erroneous_tombstones = false;
+    CompactionConfig() = default;
+    CompactionConfig(uint64_t purge_before_ts,
+                     uint64_t purge_before_seq,
+                     bool drop_deletes,
+                     bool retain_erroneous_tombstones)
+        : purge_before_ts(purge_before_ts),
+          purge_before_seq(purge_before_seq),
+          drop_deletes(drop_deletes),
+          retain_erroneous_tombstones(retain_erroneous_tombstones) {
+    }
+
+    CompactionConfig(const CompactionConfig&) = default;
+    CompactionConfig& operator=(const CompactionConfig& other) = default;
+
+    /// Move will value copy from 'other' and leave 'other' default constructed
+    CompactionConfig(CompactionConfig&& other);
+    /// Move will value copy from 'other' and leave 'other' default constructed
+    CompactionConfig& operator=(CompactionConfig&& other);
+
     bool operator==(const CompactionConfig& c) const;
     bool operator!=(const CompactionConfig& c) const {
         return !(*this == c);
     }
+
+    /**
+     * Merge 'other' into this instance. Merge results in this object being
+     * representative of the current config and the other config.
+     *
+     * - drop_deletes/retain_erroneous_tombstones are 'sticky', once true
+     *   they will remain true.
+     * - purge_before_ts and purge_before_seq become the max of this vs other
+     */
+    void merge(const CompactionConfig& other);
+
+    uint64_t purge_before_ts = 0;
+    uint64_t purge_before_seq = 0;
+    bool drop_deletes = false;
+    bool retain_erroneous_tombstones = false;
 };
 
 struct CompactionContext {

engines/ep/src/tasks.cc

@@ -120,14 +120,18 @@ std::pair<CompactionConfig, std::vector<const void*>>
 CompactTask::preDoCompact() {
     auto lockedState = compaction.wlock();
     lockedState->rescheduleRequired = false;
-    return {lockedState->config, std::move(lockedState->cookiesWaiting)};
+    // config and cookiesWaiting are moved out to the task run loop, any
+    // subsequent schedule now operates on the 'empty' objects whilst run
+    // gets on with compacting using the returned values.
+    return {std::move(lockedState->config),
+            std::move(lockedState->cookiesWaiting)};
 }
 
 void CompactTask::runCompactionWithConfig(
         std::optional<CompactionConfig> config, const void* cookie) {
     auto lockedState = compaction.wlock();
     if (config) {
-        lockedState->config = config.value();
+        lockedState->config.merge(config.value());
     }
     if (cookie) {
         lockedState->cookiesWaiting.push_back(cookie);

engines/ep/tests/module_tests/evp_store_test.cc

@@ -1671,6 +1671,40 @@ TEST_P(EPBucketTestNoRocksDb, ScheduleCompactionWithNewConfig) {
     EXPECT_FALSE(task);
 }
 
+// Test that scheduling compaction means the task which runs, runs with a merged
+// configuration that meets all requests.
+TEST_P(EPBucketTestNoRocksDb, ScheduleCompactionAndMergeNewConfig) {
+    auto* mockEPBucket = dynamic_cast<MockEPBucket*>(engine->getKVBucket());
+    // Array of configs to use for each call to schedule, it should result
+    // in a config for the run which is the 'merge of all'.
+    std::array<CompactionConfig, 5> configs = {{{0, 0, false, false},
+                                                {0, 1000, false, false},
+                                                {1000, 0, false, false},
+                                                {9, 900, false, true},
+                                                {9, 900, true, false}}};
+
+    for (const auto& config : configs) {
+        EXPECT_EQ(ENGINE_EWOULDBLOCK,
+                  mockEPBucket->scheduleCompaction(
+                          vbid, config, nullptr, std::chrono::seconds(0)));
+    }
+    auto task = mockEPBucket->getCompactionTask(vbid);
+    ASSERT_TRUE(task);
+    auto finalConfig = task->getCurrentConfig();
+
+    // Merged values, max for 'purge_before_' and true for the bools
+    EXPECT_EQ(1000, finalConfig.purge_before_ts);
+    EXPECT_EQ(1000, finalConfig.purge_before_seq);
+    EXPECT_TRUE(finalConfig.drop_deletes);
+    EXPECT_TRUE(finalConfig.retain_erroneous_tombstones);
+
+    // no reschedule needed
+    EXPECT_FALSE(task->run());
+
+    task = mockEPBucket->getCompactionTask(vbid);
+    EXPECT_FALSE(task);
+}
+
 // Test that scheduling compaction when a task is already running the task
 // will reschedule *and* the reschedule picks up the new config.
 TEST_P(EPBucketTestNoRocksDb, ScheduleCompactionReschedules) {