Simplify logic about when to call notifyRemoveDone() on gid to lid change handler.

7 files changed, 116 insertions, 51 deletions

diff --git a/searchcore/src/vespa/searchcore/proton/reference/CMakeLists.txt b/searchcore/src/vespa/searchcore/proton/reference/CMakeLists.txt
index fe2ca7a7a88..a98b095cc21 100644
--- a/searchcore/src/vespa/searchcore/proton/reference/CMakeLists.txt
+++ b/searchcore/src/vespa/searchcore/proton/reference/CMakeLists.txt
@@ -10,6 +10,7 @@ vespa_add_library(searchcore_reference STATIC
     gid_to_lid_change_registrator.cpp
     gid_to_lid_mapper.cpp
     gid_to_lid_mapper_factory.cpp
+    pending_notify_remove_done.cpp
     DEPENDS
     searchcore_attribute
     searchcore_documentmetastore
diff --git a/searchcore/src/vespa/searchcore/proton/reference/pending_notify_remove_done.cpp b/searchcore/src/vespa/searchcore/proton/reference/pending_notify_remove_done.cpp
new file mode 100644
index 00000000000..771be4684cf
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/reference/pending_notify_remove_done.cpp
@@ -0,0 +1,49 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "pending_notify_remove_done.h"
+#include <vespa/searchcore/proton/reference/i_gid_to_lid_change_handler.h>
+#include <cassert>
+
+namespace proton
+{
+
+PendingNotifyRemoveDone::PendingNotifyRemoveDone()
+    : _gidToLidChangeHandler(nullptr),
+      _gid(),
+      _serialNum(0),
+      _pending(false)
+{
+}
+
+PendingNotifyRemoveDone::PendingNotifyRemoveDone(PendingNotifyRemoveDone &&rhs)
+    : _gidToLidChangeHandler(rhs._gidToLidChangeHandler),
+      _gid(rhs._gid),
+      _serialNum(rhs._serialNum),
+      _pending(rhs._pending)
+{
+    rhs._pending = false;
+}
+
+PendingNotifyRemoveDone::~PendingNotifyRemoveDone()
+{
+    assert(!_pending); // Fail if notifyRemoveDone is still pending
+}
+
+void
+PendingNotifyRemoveDone::setup(IGidToLidChangeHandler &gidToLidChangeHandler, document::GlobalId gid, search::SerialNum serialNum) {
+    _gidToLidChangeHandler = &gidToLidChangeHandler;
+    _gid = gid;
+    _serialNum = serialNum;
+    _pending = true;
+}
+
+void
+PendingNotifyRemoveDone::invoke()
+{
+    if (_pending) {
+        _gidToLidChangeHandler->notifyRemoveDone(_gid, _serialNum);
+        _pending = false;
+    }
+}
+
+}  // namespace proton
diff --git a/searchcore/src/vespa/searchcore/proton/reference/pending_notify_remove_done.h b/searchcore/src/vespa/searchcore/proton/reference/pending_notify_remove_done.h
new file mode 100644
index 00000000000..95aad182b10
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/reference/pending_notify_remove_done.h
@@ -0,0 +1,35 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+#include <vespa/document/base/globalid.h>
+#include <vespa/searchlib/common/serialnum.h>
+
+namespace proton
+{
+
+class IGidToLidChangeHandler;
+
+/*
+ * Class used to keep track of a pending notifyRemoveDone() call to
+ * a gid to lid change handler.
+ */
+class PendingNotifyRemoveDone
+{
+    IGidToLidChangeHandler *_gidToLidChangeHandler;
+    document::GlobalId      _gid;
+    search::SerialNum       _serialNum;
+    bool                    _pending;
+
+public:
+    PendingNotifyRemoveDone();
+    PendingNotifyRemoveDone(PendingNotifyRemoveDone &&rhs);
+    PendingNotifyRemoveDone(const PendingNotifyRemoveDone &rhs) = delete;
+    PendingNotifyRemoveDone &operator=(const PendingNotifyRemoveDone &rhs) = delete;
+    PendingNotifyRemoveDone &operator=(PendingNotifyRemoveDone &&rhs) = delete;
+    ~PendingNotifyRemoveDone();
+    void setup(IGidToLidChangeHandler &gidToLidChangeHandler, document::GlobalId gid, search::SerialNum serialNum);
+    void invoke();
+};
+
+}  // namespace proton
diff --git a/searchcore/src/vespa/searchcore/proton/server/removedonecontext.cpp b/searchcore/src/vespa/searchcore/proton/server/removedonecontext.cpp
index 522b0aed617..627e8d9f627 100644
--- a/searchcore/src/vespa/searchcore/proton/server/removedonecontext.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/removedonecontext.cpp
@@ -12,18 +12,12 @@ RemoveDoneContext::RemoveDoneContext(std::unique_ptr<FeedToken> token,
                                      PerDocTypeFeedMetrics &metrics,
                                      vespalib::Executor &executor,
                                      IDocumentMetaStore &documentMetaStore,
-                                     IGidToLidChangeHandler &gidToLidChangeHandler,
-                                     const document::GlobalId &gid,
-                                     uint32_t lid,
-                                     search::SerialNum serialNum,
-                                     bool enableNotifyRemoveDone)
+                                     PendingNotifyRemoveDone &&pendingNotifyRemoveDone,
+                                     uint32_t lid)
     : OperationDoneContext(std::move(token), opType, metrics),
       _executor(executor),
       _task(),
-      _gidToLidChangeHandler(gidToLidChangeHandler),
-      _gid(gid),
-      _serialNum(serialNum),
-      _enableNotifyRemoveDone(enableNotifyRemoveDone)
+      _pendingNotifyRemoveDone(std::move(pendingNotifyRemoveDone))
 {
     if (lid != 0) {
         _task = std::make_unique<RemoveDoneTask>(documentMetaStore, lid);
@@ -32,9 +26,7 @@ RemoveDoneContext::RemoveDoneContext(std::unique_ptr<FeedToken> token,
 
 RemoveDoneContext::~RemoveDoneContext()
 {
-    if (_enableNotifyRemoveDone) {
-        _gidToLidChangeHandler.notifyRemoveDone(_gid, _serialNum);
-    }
+    _pendingNotifyRemoveDone.invoke();
     ack();
     if (_task) {
         vespalib::Executor::Task::UP res = _executor.execute(std::move(_task));
diff --git a/searchcore/src/vespa/searchcore/proton/server/removedonecontext.h b/searchcore/src/vespa/searchcore/proton/server/removedonecontext.h
index 9311a6d2b6e..c4fafb4e886 100644
--- a/searchcore/src/vespa/searchcore/proton/server/removedonecontext.h
+++ b/searchcore/src/vespa/searchcore/proton/server/removedonecontext.h
@@ -6,12 +6,12 @@
 #include <vespa/vespalib/util/executor.h>
 #include <vespa/document/base/globalid.h>
 #include <vespa/searchlib/common/serialnum.h>
+#include <vespa/searchcore/proton/reference/pending_notify_remove_done.h>
 
 namespace proton
 {
 
 class IDocumentMetaStore;
-class IGidToLidChangeHandler;
 
 
 /**
@@ -26,10 +26,7 @@ class RemoveDoneContext : public OperationDoneContext
 {
     vespalib::Executor &_executor;
     std::unique_ptr<vespalib::Executor::Task> _task;
-    IGidToLidChangeHandler &_gidToLidChangeHandler;
-    document::GlobalId _gid;
-    search::SerialNum _serialNum;
-    bool _enableNotifyRemoveDone;
+    PendingNotifyRemoveDone _pendingNotifyRemoveDone;
 
 public:
     RemoveDoneContext(std::unique_ptr<FeedToken> token,
@@ -37,11 +34,8 @@ public:
                       PerDocTypeFeedMetrics &metrics,
                       vespalib::Executor &executor,
                       IDocumentMetaStore &documentMetaStore,
-                      IGidToLidChangeHandler &gidToLidChangeHandler,
-                      const document::GlobalId &gid,
-                      uint32_t lid,
-                      search::SerialNum serialNum,
-                      bool enableNotifyRemoveDone);
+                      PendingNotifyRemoveDone &&pendingNotifyRemoveDone,
+                      uint32_t lid);
 
     virtual ~RemoveDoneContext();
 };
diff --git a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
index 5f0fc3b8aa6..489c3fc2591 100644
--- a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
@@ -117,14 +117,11 @@ public:
     RemoveDoneContextForMove(std::unique_ptr<FeedToken> token, const FeedOperation::Type opType,
                              PerDocTypeFeedMetrics &metrics, vespalib::Executor &executor,
                              IDocumentMetaStore &documentMetaStore,
-                             IGidToLidChangeHandler &gidToLidChangeHandler,
-                             const document::GlobalId &gid,
+                             PendingNotifyRemoveDone &&pendingNotifyRemoveDone,
                              uint32_t lid,
-                             SerialNum serialNum,
-                             bool enableNotifyRemoveDone,
                              IDestructorCallback::SP moveDoneCtx)
-        : RemoveDoneContext(std::move(token), opType, metrics, executor, documentMetaStore, gidToLidChangeHandler, gid, lid, serialNum, enableNotifyRemoveDone),
-              _moveDoneCtx(std::move(moveDoneCtx))
+        : RemoveDoneContext(std::move(token), opType, metrics, executor, documentMetaStore, std::move(pendingNotifyRemoveDone) ,lid),
+          _moveDoneCtx(std::move(moveDoneCtx))
     {}
     virtual ~RemoveDoneContextForMove() {}
 };
@@ -133,19 +130,16 @@ std::shared_ptr<RemoveDoneContext>
 createRemoveDoneContext(std::unique_ptr<FeedToken> token, const FeedOperation::Type opType,
                         PerDocTypeFeedMetrics &metrics, vespalib::Executor &executor,
                         IDocumentMetaStore &documentMetaStore,
-                        IGidToLidChangeHandler &gidToLidChangeHandler,
-                        const document::GlobalId &gid,
+                        PendingNotifyRemoveDone &&pendingNotifyRemoveDone,
                         uint32_t lid,
-                        SerialNum serialNum,
-                        bool enableNotifyRemoveDone,
                         IDestructorCallback::SP moveDoneCtx)
 {
     if (moveDoneCtx) {
         return std::make_shared<RemoveDoneContextForMove>
-            (std::move(token), opType, metrics, executor, documentMetaStore, gidToLidChangeHandler, gid, lid, serialNum, enableNotifyRemoveDone, std::move(moveDoneCtx));
+            (std::move(token), opType, metrics, executor, documentMetaStore, std::move(pendingNotifyRemoveDone), lid, std::move(moveDoneCtx));
     } else {
         return std::make_shared<RemoveDoneContext>
-            (std::move(token), opType, metrics, executor, documentMetaStore, gidToLidChangeHandler, gid, lid, serialNum, enableNotifyRemoveDone);
+            (std::move(token), opType, metrics, executor, documentMetaStore, std::move(pendingNotifyRemoveDone), lid);
     }
 }
 
@@ -298,7 +292,7 @@ StoreOnlyFeedView::internalPut(FeedToken::UP token, const PutOperation &putOp)
          putOp.getSubDbId(), putOp.getLid(), putOp.getPrevSubDbId(), putOp.getPrevLid(),
          _params._subDbId, doc->toString(true).size(), doc->toString(true).c_str());
 
-    adjustMetaStore(putOp, docId);
+    PendingNotifyRemoveDone pendingNotifyRemoveDone = adjustMetaStore(putOp, docId);
     considerEarlyAck(token, putOp.getType());
 
     bool docAlreadyExists = putOp.getValidPrevDbdId(_params._subDbId);
@@ -315,8 +309,7 @@ StoreOnlyFeedView::internalPut(FeedToken::UP token, const PutOperation &putOp)
     }
     if (docAlreadyExists && putOp.changedDbdId()) {
         assert(!putOp.getValidDbdId(_params._subDbId));
-        const document::GlobalId &gid = docId.getGlobalId();
-        internalRemove(std::move(token), serialNum, gid, putOp.getPrevLid(), putOp.getType(), useDocumentMetaStore(serialNum), IDestructorCallback::SP());
+        internalRemove(std::move(token), serialNum, std::move(pendingNotifyRemoveDone), putOp.getPrevLid(), putOp.getType(), IDestructorCallback::SP());
     }
     if (token.get() != NULL) {
         token->ack(putOp.getType(), _params._metrics);
@@ -581,7 +574,7 @@ StoreOnlyFeedView::internalRemove(FeedToken::UP token, const RemoveOperation &rm
          _params._docTypeName.toString().c_str(), serialNum, docId.toString().c_str(),
          rmOp.getSubDbId(), rmOp.getLid(), rmOp.getPrevSubDbId(), rmOp.getPrevLid(), _params._subDbId);
 
-    adjustMetaStore(rmOp, docId);
+    PendingNotifyRemoveDone pendingNotifyRemoveDone = adjustMetaStore(rmOp, docId);
     considerEarlyAck(token, rmOp.getType());
 
     if (rmOp.getValidDbdId(_params._subDbId)) {
@@ -593,8 +586,7 @@ StoreOnlyFeedView::internalRemove(FeedToken::UP token, const RemoveOperation &rm
     if (rmOp.getValidPrevDbdId(_params._subDbId)) {
         if (rmOp.changedDbdId()) {
             assert(!rmOp.getValidDbdId(_params._subDbId));
-            const document::GlobalId &gid = docId.getGlobalId();
-            internalRemove(std::move(token), serialNum, gid, rmOp.getPrevLid(), rmOp.getType(), useDocumentMetaStore(serialNum), IDestructorCallback::SP());
+            internalRemove(std::move(token), serialNum, std::move(pendingNotifyRemoveDone), rmOp.getPrevLid(), rmOp.getType(), IDestructorCallback::SP());
         }
     }
     if (token.get() != NULL) {
@@ -603,22 +595,23 @@ StoreOnlyFeedView::internalRemove(FeedToken::UP token, const RemoveOperation &rm
 }
 
 void
-StoreOnlyFeedView::internalRemove(FeedToken::UP token, SerialNum serialNum, const document::GlobalId &gid, Lid lid,
-                                  FeedOperation::Type opType, bool enableNotifyRemoveDone, IDestructorCallback::SP moveDoneCtx)
+StoreOnlyFeedView::internalRemove(FeedToken::UP token, SerialNum serialNum, PendingNotifyRemoveDone &&pendingNotifyRemoveDone, Lid lid,
+                                  FeedOperation::Type opType, IDestructorCallback::SP moveDoneCtx)
 {
     removeSummary(serialNum, lid);
     bool explicitReuseLid = _lidReuseDelayer.delayReuse(lid);
     std::shared_ptr<RemoveDoneContext> onWriteDone;
     onWriteDone = createRemoveDoneContext(std::move(token), opType, _params._metrics, _writeService.master(),
-                                          _metaStore, _gidToLidChangeHandler, gid, (explicitReuseLid ? lid : 0u), serialNum, enableNotifyRemoveDone, moveDoneCtx);
+                                          _metaStore, std::move(pendingNotifyRemoveDone), (explicitReuseLid ? lid : 0u), moveDoneCtx);
     bool immediateCommit = _commitTimeTracker.needCommit();
     removeAttributes(serialNum, lid, immediateCommit, onWriteDone);
     removeIndexedFields(serialNum, lid, immediateCommit, onWriteDone);
 }
 
-void
+PendingNotifyRemoveDone
 StoreOnlyFeedView::adjustMetaStore(const DocumentOperation &op, const DocumentId &docId)
 {
+    PendingNotifyRemoveDone pendingNotifyRemoveDone;
     const SerialNum serialNum = op.getSerialNum();
     if (useDocumentMetaStore(serialNum)) {
         if (op.getValidDbdId(_params._subDbId)) {
@@ -632,10 +625,12 @@ StoreOnlyFeedView::adjustMetaStore(const DocumentOperation &op, const DocumentId
             }
         } else if (op.getValidPrevDbdId(_params._subDbId)) {
             _gidToLidChangeHandler.notifyRemove(docId.getGlobalId(), serialNum);
+            pendingNotifyRemoveDone.setup(_gidToLidChangeHandler, docId.getGlobalId(), serialNum);
             removeMetaData(_metaStore, docId, op, _params._subDbType == SubDbType::REMOVED);
         }
         _metaStore.commit(serialNum, serialNum);
     }
+    return pendingNotifyRemoveDone;
 }
 
 void
@@ -750,7 +745,7 @@ StoreOnlyFeedView::handleMove(const MoveOperation &moveOp, IDestructorCallback::
          moveOp.getSubDbId(), moveOp.getLid(), moveOp.getPrevSubDbId(), moveOp.getPrevLid(),
          _params._subDbId, doc->toString(true).size(), doc->toString(true).c_str());
 
-    adjustMetaStore(moveOp, docId);
+    PendingNotifyRemoveDone pendingNotifyRemoveDone = adjustMetaStore(moveOp, docId);
     bool docAlreadyExists = moveOp.getValidPrevDbdId(_params._subDbId);
     if (moveOp.getValidDbdId(_params._subDbId)) {
         bool immediateCommit = _commitTimeTracker.needCommit();
@@ -765,9 +760,7 @@ StoreOnlyFeedView::handleMove(const MoveOperation &moveOp, IDestructorCallback::
         putIndexedFields(serialNum, moveOp.getLid(), doc, immediateCommit, onWriteDone);
     }
     if (docAlreadyExists && moveOp.changedDbdId()) {
-        const document::GlobalId &gid = docId.getGlobalId();
-        bool enableNotifyRemoveDone = useDocumentMetaStore(serialNum) && !moveOp.getValidDbdId(_params._subDbId);
-        internalRemove(FeedToken::UP(), serialNum, gid, moveOp.getPrevLid(), moveOp.getType(), enableNotifyRemoveDone, doneCtx);
+        internalRemove(FeedToken::UP(), serialNum, std::move(pendingNotifyRemoveDone), moveOp.getPrevLid(), moveOp.getType(), doneCtx);
     }
 }
 
diff --git a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h
index 01a8122ed1e..edb36350ed9 100644
--- a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h
+++ b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.h
@@ -15,6 +15,7 @@
 #include <vespa/searchcore/proton/documentmetastore/documentmetastorecontext.h>
 #include <vespa/searchcore/proton/feedoperation/feedoperation.h>
 #include <vespa/searchcore/proton/persistenceengine/resulthandler.h>
+#include <vespa/searchcore/proton/reference/pending_notify_remove_done.h>
 #include <vespa/searchcorespi/index/ithreadingservice.h>
 #include <vespa/searchlib/query/base.h>
 #include <vespa/vespalib/util/threadstackexecutorbase.h>
@@ -168,7 +169,7 @@ private:
         return replaySerialNum > _params._flushedDocumentMetaStoreSerialNum;
     }
 
-    void adjustMetaStore(const DocumentOperation &op, const document::DocumentId &docId);
+    PendingNotifyRemoveDone adjustMetaStore(const DocumentOperation &op, const document::DocumentId &docId);
     void internalPut(FeedTokenUP token, const PutOperation &putOp);
     void internalUpdate(FeedTokenUP token, const UpdateOperation &updOp);
 
@@ -180,8 +181,8 @@ private:
     size_t removeDocuments(const RemoveDocumentsOperation &op, bool remove_index_and_attribute_fields,
                            bool immediateCommit);
 
-    void internalRemove(FeedTokenUP token, SerialNum serialNum, const document::GlobalId &gid, Lid lid,
-                        FeedOperation::Type opType, bool enableNotifyRemoveDone, std::shared_ptr<search::IDestructorCallback> moveDoneCtx);
+    void internalRemove(FeedTokenUP token, SerialNum serialNum, PendingNotifyRemoveDone &&pendingNotifyRemoveDone, Lid lid,
+                        FeedOperation::Type opType, std::shared_ptr<search::IDestructorCallback> moveDoneCtx);
 
     // Ack token early if visibility delay is nonzero
     void considerEarlyAck(FeedTokenUP &token, FeedOperation::Type opType);