MB-35629: Revert "MB-35458 [SR]: Move SyncWrite completion to bg Dura…

…bilityCompletionTask" The introduction of the background commit has introduced a (severe) performance regression - SyncWrites are getting stuck and timing out. Suspect an issue with the wakeup / notificaiotn of the DurabilityCompletionTask; reverting change to restore performance while investigating. This reverts commit 7fdb98a. Change-Id: I8d08044d3f6ad3084f7724ead961b8d7530006f1 Reviewed-on: http://review.couchbase.org/113637 Reviewed-by: Ben Huddleston <[email protected]> Tested-by: Build Bot <[email protected]>
couchbase · Aug 21, 2019 · e39b54f · e39b54f
1 parent 8ba5a89
commit e39b54f
Show file tree

Hide file tree

Showing 35 changed files with 75 additions and 455 deletions.
diff --git a/engines/ep/CMakeLists.txt b/engines/ep/CMakeLists.txt
@@ -227,7 +227,6 @@ ADD_LIBRARY(ep_objs OBJECT
             src/defragmenter_visitor.cc
             src/diskdockey.cc
             src/durability/active_durability_monitor.cc
-            src/durability/durability_completion_task.cc
             src/durability/durability_monitor.cc
             src/durability/durability_monitor_impl.cc
             src/durability/passive_durability_monitor.cc

diff --git a/engines/ep/benchmarks/defragmenter_bench.cc b/engines/ep/benchmarks/defragmenter_bench.cc
@@ -56,7 +56,6 @@ class DefragmentBench : public benchmark::Fixture {
                               /*table*/ nullptr,
                               std::make_shared<DummyCB>(),
                               /*newSeqnoCb*/ nullptr,
-                              [](Vbid) { return; },
                               NoopSyncWriteCompleteCb,
                               NoopSeqnoAckCb,
                               config,

diff --git a/engines/ep/benchmarks/item_compressor_bench.cc b/engines/ep/benchmarks/item_compressor_bench.cc
@@ -55,7 +55,6 @@ class ItemCompressorBench : public benchmark::Fixture {
                               /*table*/ nullptr,
                               std::make_shared<DummyCB>(),
                               /*newSeqnoCb*/ nullptr,
-                              [](Vbid) { return; },
                               NoopSyncWriteCompleteCb,
                               NoopSeqnoAckCb,
                               config,

diff --git a/engines/ep/src/durability/active_durability_monitor.cc b/engines/ep/src/durability/active_durability_monitor.cc
@@ -544,7 +544,7 @@ void ActiveDurabilityMonitor::setReplicationTopology(
         s->setReplicationTopology(topology, *resolvedQueue);
     }
 
-    checkForResolvedSyncWrites();
+    processCompletedSyncWriteQueue();
 }
 
 int64_t ActiveDurabilityMonitor::getHighPreparedSeqno() const {
@@ -619,9 +619,8 @@ ENGINE_ERROR_CODE ActiveDurabilityMonitor::seqnoAckReceived(
         seqnoAckReceivedPostProcessHook();
     }
 
-    // Check if any there's now any resolved SyncWrites which should be
-    // completed.
-    checkForResolvedSyncWrites();
+    // Process the Completed Queue, committing all items and removing them.
+    processCompletedSyncWriteQueue();
 
     return ENGINE_SUCCESS;
 }
@@ -640,7 +639,7 @@ void ActiveDurabilityMonitor::processTimeout(
     // the correct locks).
     state.wlock()->removeExpired(asOf, *resolvedQueue);
 
-    checkForResolvedSyncWrites();
+    processCompletedSyncWriteQueue();
 }
 
 void ActiveDurabilityMonitor::notifyLocalPersistence() {
@@ -729,13 +728,6 @@ void ActiveDurabilityMonitor::addStatsForChain(
     }
 }
 
-void ActiveDurabilityMonitor::checkForResolvedSyncWrites() {
-    if (resolvedQueue->empty()) {
-        return;
-    }
-    vb.notifySyncWritesPendingCompletion();
-}
-
 void ActiveDurabilityMonitor::processCompletedSyncWriteQueue() {
     std::lock_guard<ResolvedQueue::ConsumerLock> lock(
             resolvedQueue->getConsumerLock());
@@ -1680,7 +1672,11 @@ void ActiveDurabilityMonitor::checkForCommit() {
     // the resolvedQueue (under the correct locks).
     state.wlock()->updateHighPreparedSeqno(*resolvedQueue);
 
-    checkForResolvedSyncWrites();
+    // @todo: Consider to commit in a dedicated function for minimizing
+    //     contention on front-end threads, as this function is supposed to
+    //     execute under VBucket-level lock.
+
+    processCompletedSyncWriteQueue();
 }
 
 template <class exception>

diff --git a/engines/ep/src/durability/active_durability_monitor.h b/engines/ep/src/durability/active_durability_monitor.h
@@ -282,12 +282,6 @@ class ActiveDurabilityMonitor : public DurabilityMonitor {
      */
     void removedQueuedAck(const std::string& node);
 
-    /**
-     * For all items in the completedSWQueue, call VBucket::commit /
-     * VBucket::abort as appropriate, then remove the item from the queue.
-     */
-    void processCompletedSyncWriteQueue();
-
     /**
      * @return all of the currently tracked writes
      */
@@ -369,10 +363,10 @@ class ActiveDurabilityMonitor : public DurabilityMonitor {
                           const ReplicationChain& chain) const;
 
     /**
-     * Checks if the resolvedQueue contains any SyncWrites awaiting completion,
-     * and if so notifies the VBucket.
+     * For all items in the completedSWQueue, call VBucket::commit /
+     * VBucket::abort as appropriate, then remove the item from the queue.
      */
-    void checkForResolvedSyncWrites();
+    void processCompletedSyncWriteQueue();
 
     // The stats object for the owning Bucket
     EPStats& stats;

diff --git a/engines/ep/src/durability/durability_completion_task.cc b/engines/ep/src/durability/durability_completion_task.cc
diff --git a/engines/ep/src/durability/durability_completion_task.h b/engines/ep/src/durability/durability_completion_task.h
diff --git a/engines/ep/src/ep_bucket.cc b/engines/ep/src/ep_bucket.cc
@@ -1138,7 +1138,6 @@ VBucketPtr EPBucket::makeVBucket(
                                     std::move(table),
                                     flusherCb,
                                     std::move(newSeqnoCb),
-                                    makeSyncWriteResolvedCB(),
                                     makeSyncWriteCompleteCB(),
                                     makeSeqnoAckCB(),
                                     engine.getConfiguration(),

diff --git a/engines/ep/src/ep_vb.cc b/engines/ep/src/ep_vb.cc
@@ -47,7 +47,6 @@ EPVBucket::EPVBucket(Vbid i,
                      std::unique_ptr<FailoverTable> table,
                      std::shared_ptr<Callback<Vbid>> flusherCb,
                      NewSeqnoCallback newSeqnoCb,
-                     SyncWriteResolvedCallback syncWriteResolvedCb,
                      SyncWriteCompleteCallback syncWriteCb,
                      SeqnoAckCallback seqnoAckCb,
                      Configuration& config,
@@ -70,7 +69,6 @@ EPVBucket::EPVBucket(Vbid i,
               flusherCb,
               std::make_unique<StoredValueFactory>(st),
               std::move(newSeqnoCb),
-              syncWriteResolvedCb,
               syncWriteCb,
               seqnoAckCb,
               config,

diff --git a/engines/ep/src/ep_vb.h b/engines/ep/src/ep_vb.h
@@ -39,7 +39,6 @@ class EPVBucket : public VBucket {
               std::unique_ptr<FailoverTable> table,
               std::shared_ptr<Callback<Vbid>> flusherCb,
               NewSeqnoCallback newSeqnoCb,
-              SyncWriteResolvedCallback syncWriteResolvedCb,
               SyncWriteCompleteCallback syncWriteCb,
               SeqnoAckCallback seqnoAckCb,
               Configuration& config,

diff --git a/engines/ep/src/ephemeral_bucket.cc b/engines/ep/src/ephemeral_bucket.cc
@@ -208,7 +208,6 @@ VBucketPtr EphemeralBucket::makeVBucket(
                                            lastSnapEnd,
                                            std::move(table),
                                            std::move(newSeqnoCb),
-                                           makeSyncWriteResolvedCB(),
                                            makeSyncWriteCompleteCB(),
                                            makeSeqnoAckCB(),
                                            engine.getConfiguration(),

diff --git a/engines/ep/src/ephemeral_vb.cc b/engines/ep/src/ephemeral_vb.cc
@@ -44,7 +44,6 @@ EphemeralVBucket::EphemeralVBucket(
         uint64_t lastSnapEnd,
         std::unique_ptr<FailoverTable> table,
         NewSeqnoCallback newSeqnoCb,
-        SyncWriteResolvedCallback syncWriteResolvedCb,
         SyncWriteCompleteCallback syncWriteCb,
         SeqnoAckCallback seqnoAckCb,
         Configuration& config,
@@ -66,7 +65,6 @@ EphemeralVBucket::EphemeralVBucket(
               /*flusherCb*/ nullptr,
               std::make_unique<OrderedStoredValueFactory>(st),
               std::move(newSeqnoCb),
-              syncWriteResolvedCb,
               syncWriteCb,
               seqnoAckCb,
               config,

diff --git a/engines/ep/src/ephemeral_vb.h b/engines/ep/src/ephemeral_vb.h
@@ -39,7 +39,6 @@ class EphemeralVBucket : public VBucket {
                      uint64_t lastSnapEnd,
                      std::unique_ptr<FailoverTable> table,
                      NewSeqnoCallback newSeqnoCb,
-                     SyncWriteResolvedCallback syncWriteResolvedCb,
                      SyncWriteCompleteCallback syncWriteCb,
                      SeqnoAckCallback seqnoAckCb,
                      Configuration& config,

diff --git a/engines/ep/src/kv_bucket.cc b/engines/ep/src/kv_bucket.cc
@@ -39,7 +39,6 @@
 #include "connmap.h"
 #include "dcp/dcpconnmap.h"
 #include "defragmenter.h"
-#include "durability/durability_completion_task.h"
 #include "durability_timeout_task.h"
 #include "ep_engine.h"
 #include "ep_time.h"
@@ -451,10 +450,6 @@ bool KVBucket::initialize() {
                     config.getDurabilityTimeoutTaskInterval()));
     ExecutorPool::get()->schedule(durabilityTimeoutTask);
 
-    durabilityCompletionTask =
-            std::make_shared<DurabilityCompletionTask>(engine);
-    ExecutorPool::get()->schedule(durabilityCompletionTask);
-
     ExTask workloadMonitorTask =
             std::make_shared<WorkLoadMonitor>(&engine, false);
     ExecutorPool::get()->schedule(workloadMonitorTask);
@@ -2630,14 +2625,6 @@ uint16_t KVBucket::getNumOfVBucketsInState(vbucket_state_t state) const {
     return vbMap.getVBStateCount(state);
 }
 
-SyncWriteResolvedCallback KVBucket::makeSyncWriteResolvedCB() {
-    return [this](Vbid vbid) {
-        if (this->durabilityCompletionTask) {
-            this->durabilityCompletionTask->notifySyncWritesToComplete(vbid);
-        }
-    };
-}
-
 SyncWriteCompleteCallback KVBucket::makeSyncWriteCompleteCB() {
     auto& engine = this->engine;
     return [&engine](const void* cookie, ENGINE_ERROR_CODE status) {