Test that documents failing move are detected and causes retry and eventual completition.

5 files changed, 77 insertions, 31 deletions

diff --git a/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.cpp b/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.cpp
index ff44d898997..5c4cfb393a4 100644
--- a/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.cpp
+++ b/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.cpp
@@ -21,7 +21,9 @@ MyBucketModifiedHandler::notifyBucketModified(const BucketId &bucket) {
 MyMoveHandler::MyMoveHandler(bucketdb::BucketDBOwner &bucketDb, bool storeMoveDoneContext)
     : _bucketDb(bucketDb),
       _moves(),
-      _numCachedBuckets(),
+      _lids2Fail(),
+      _numFailedMoves(0),
+      _numCachedBuckets(0),
       _storeMoveDoneContexts(storeMoveDoneContext),
       _moveDoneContexts()
 {}
@@ -30,6 +32,10 @@ MyMoveHandler::~MyMoveHandler() = default;
 
 IDocumentMoveHandler::MoveResult
 MyMoveHandler::handleMove(MoveOperation &op, IDestructorCallback::SP moveDoneCtx) {
+    if (_lids2Fail.contains(op.getPrevLid())) {
+        _numFailedMoves++;
+        return MoveResult::FAILURE;
+    }
     _moves.push_back(op);
     if (_bucketDb.takeGuard()->isCachedBucket(op.getBucketId())) {
         ++_numCachedBuckets;
@@ -71,6 +77,13 @@ MySubDb::insertDocs(const UserDocuments &docs_) {
 }
 
 bool
+MySubDb::remove(uint32_t subDbId, uint32_t lid) {
+    if (_subDb.sub_db_id() != subDbId) return false;
+    if (!_metaStore.validLid(lid)) return false;
+    return _metaStore.remove(lid, 0u);
+}
+
+bool
 assertEqual(const document::BucketId &bucket, const proton::test::Document &doc,
             uint32_t sourceSubDbId, uint32_t targetSubDbId, const MoveOperation &op) {
     if (!EXPECT_EQUAL(bucket, op.getBucketId())) return false;
diff --git a/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.h b/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.h
index ded00589f60..e21f084ff6e 100644
--- a/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.h
+++ b/searchcore/src/tests/proton/documentdb/documentbucketmover/bucketmover_common.h
@@ -37,14 +37,25 @@ struct MyMoveHandler : public IDocumentMoveHandler {
     using MoveOperationVector = std::vector<MoveOperation>;
     bucketdb::BucketDBOwner &_bucketDb;
     MoveOperationVector _moves;
+    std::set<uint32_t> _lids2Fail;
+    size_t _numFailedMoves;
     size_t _numCachedBuckets;
-    bool _storeMoveDoneContexts;
+    bool   _storeMoveDoneContexts;
+
     std::vector<vespalib::IDestructorCallback::SP> _moveDoneContexts;
 
-    MyMoveHandler(bucketdb::BucketDBOwner &bucketDb, bool storeMoveDoneContext = false);
+    explicit MyMoveHandler(bucketdb::BucketDBOwner &bucketDb) : MyMoveHandler(bucketDb, false){}
+    MyMoveHandler(bucketdb::BucketDBOwner &bucketDb, bool storeMoveDoneContext);
     ~MyMoveHandler() override;
     MoveResult handleMove(MoveOperation &op, vespalib::IDestructorCallback::SP moveDoneCtx) override;
 
+    void addLid2Fail(uint32_t lid) {
+        _lids2Fail.insert(lid);
+    }
+    void removeLids2Fail(uint32_t lid) {
+        _lids2Fail.erase(lid);
+    }
+
     void reset() {
         _moves.clear();
         _numCachedBuckets = 0;
@@ -66,7 +77,7 @@ struct MyDocumentRetriever : public DocumentRetrieverBaseForTest {
     DocumentVector _docs;
     uint32_t _lid2Fail;
 
-    MyDocumentRetriever(std::shared_ptr<const DocumentTypeRepo> repo)
+    explicit MyDocumentRetriever(std::shared_ptr<const DocumentTypeRepo> repo)
         : _repo(std::move(repo)),
           _docs(),
           _lid2Fail(0)
@@ -78,7 +89,7 @@ struct MyDocumentRetriever : public DocumentRetrieverBaseForTest {
 
     void getBucketMetaData(const storage::spi::Bucket &, DocumentMetaData::Vector &) const override {}
 
-    DocumentMetaData getDocumentMetaData(const DocumentId &) const override { return DocumentMetaData(); }
+    DocumentMetaData getDocumentMetaData(const DocumentId &) const override { return {}; }
 
     Document::UP getFullDocument(DocumentIdT lid) const override {
         return (lid != _lid2Fail) ? Document::UP(_docs[lid]->clone()) : Document::UP();
@@ -130,11 +141,13 @@ struct MySubDb {
         return _docs.getBucket(userId);
     }
 
-    DocumentVector docs(uint32_t userId) {
+    DocumentVector docs(uint32_t userId) const {
         return _docs.getGidOrderDocs(userId);
     }
 
     void setBucketState(const BucketId &bucketId, bool active);
+    // Will remove from metastore so it is invisible.
+    bool remove(uint32_t subDbId, uint32_t lid);
 };
 
 struct MyCountJobRunner : public IMaintenanceJobRunner {
diff --git a/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp b/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
index 3f4fd0c1a8f..e9407d5445e 100644
--- a/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
@@ -75,6 +75,9 @@ struct ControllerFixtureBase : public ::testing::Test
         _bucketHandler.notifyBucketStateChanged(bucket, BucketInfo::ActiveState::NOT_ACTIVE);
         return *this;
     }
+    bool removeFromSubDB(uint32_t subdbId, uint32_t lid) {
+        return _ready.remove(subdbId, lid) || _notReady.remove(subdbId, lid);
+    }
     void failRetrieveForLid(uint32_t lid) {
         _ready.failRetrieveForLid(lid);
         _notReady.failRetrieveForLid(lid);
@@ -237,6 +240,7 @@ TEST_F(ControllerFixture, require_that_bucket_is_moved_even_with_error)
     });
     sync();
     EXPECT_FALSE(_bmj->done());
+    EXPECT_TRUE(docsMoved().empty());
     fixRetriever();
     masterExecute([this]() {
         EXPECT_TRUE(_bmj->scanAndMove(4, 3));
@@ -250,6 +254,38 @@ TEST_F(ControllerFixture, require_that_bucket_is_moved_even_with_error)
     EXPECT_EQ(_ready.bucket(2), bucketsModified()[0]);
 }
 
+TEST_F(ControllerFixture, require_that_bucket_is_moved_even_with_handler_error)
+{
+    // bucket 2 should be moved
+    addReady(_ready.bucket(1));
+    _bmj->recompute();
+    _moveHandler.addLid2Fail(5);
+    masterExecute([this]() {
+        EXPECT_FALSE(_bmj->done());
+        EXPECT_TRUE(_bmj->scanAndMove(4, 3));
+        EXPECT_TRUE(_bmj->done());
+    });
+    sync();
+    EXPECT_FALSE(_bmj->done());
+    EXPECT_EQ(1u, _moveHandler._numFailedMoves);
+    EXPECT_EQ(1u, docsMoved().size());
+    assertEqual(_ready.bucket(2), _ready.docs(2)[1], 1, 2, docsMoved()[0]);
+    const auto & doc = docsMoved()[0];
+    EXPECT_TRUE(removeFromSubDB(doc.getPrevSubDbId(), doc.getPrevLid()));  // Explicit remove as movehandler only observes move attempt.
+    _moveHandler.removeLids2Fail(5);
+    masterExecute([this]() {
+        EXPECT_TRUE(_bmj->scanAndMove(4, 3));
+        EXPECT_TRUE(_bmj->done());
+    });
+    sync();
+    EXPECT_EQ(2u, docsMoved().size());
+    // First document is from previous move round. Handler just appends all operations.
+    assertEqual(_ready.bucket(2), _ready.docs(2)[1], 1, 2, docsMoved()[0]);
+    assertEqual(_ready.bucket(2), _ready.docs(2)[0], 1, 2, docsMoved()[1]);
+    EXPECT_EQ(1u, bucketsModified().size());
+    EXPECT_EQ(_ready.bucket(2), bucketsModified()[0]);
+}
+
 
 TEST_F(ControllerFixture, require_that_we_move_buckets_in_several_steps)
 {
diff --git a/searchcore/src/vespa/searchcore/proton/test/buckethandler.cpp b/searchcore/src/vespa/searchcore/proton/test/buckethandler.cpp
index 815c9741c07..52acb059a97 100644
--- a/searchcore/src/vespa/searchcore/proton/test/buckethandler.cpp
+++ b/searchcore/src/vespa/searchcore/proton/test/buckethandler.cpp
@@ -23,16 +23,14 @@ BucketHandler::addBucketStateChangedHandler(IBucketStateChangedHandler *handler)
 }
 
 void
-BucketHandler::removeBucketStateChangedHandler(IBucketStateChangedHandler *
-                                               handler)
+BucketHandler::removeBucketStateChangedHandler(IBucketStateChangedHandler * handler)
 {
     _handlers.erase(handler);
 }
 
 void
 BucketHandler::notifyBucketStateChanged(const document::BucketId &bucketId,
-                                        storage::spi::BucketInfo::ActiveState
-                                        newState)
+                                        storage::spi::BucketInfo::ActiveState newState)
 {
     for (auto &handler : _handlers) {
         handler->notifyBucketStateChanged(bucketId, newState);
diff --git a/searchcore/src/vespa/searchcore/proton/test/buckethandler.h b/searchcore/src/vespa/searchcore/proton/test/buckethandler.h
index 47dc3145f72..54f721fad86 100644
--- a/searchcore/src/vespa/searchcore/proton/test/buckethandler.h
+++ b/searchcore/src/vespa/searchcore/proton/test/buckethandler.h
@@ -6,11 +6,7 @@
 #include <vespa/searchcore/proton/server/ibucketstatechangedhandler.h>
 #include <set>
 
-namespace proton
-{
-
-namespace test
-{
+namespace proton::test {
 
 /**
  * Test bucket handler that forwards bucket state change notifications
@@ -21,22 +17,12 @@ class BucketHandler : public IBucketStateChangedNotifier
     std::set<IBucketStateChangedHandler *> _handlers;
 public:
     BucketHandler();
+    ~BucketHandler() override;
+    void addBucketStateChangedHandler(IBucketStateChangedHandler *handler) override;
+    void removeBucketStateChangedHandler(IBucketStateChangedHandler *handler) override;
 
-    virtual
-    ~BucketHandler();
-
-    virtual void
-    addBucketStateChangedHandler(IBucketStateChangedHandler *handler) override;
-
-    virtual void
-    removeBucketStateChangedHandler(IBucketStateChangedHandler *handler) override;
-
-    void
-    notifyBucketStateChanged(const document::BucketId &bucketId,
-                             storage::spi::BucketInfo::ActiveState newState);
+    void notifyBucketStateChanged(const document::BucketId &bucketId,
+                                  storage::spi::BucketInfo::ActiveState newState);
 };
 
-
-} // namespace test
-
-} // namespace proton
+}