Add explicit test that active, not-ready buckets are not moved unless bucketstate calculator says so.

5 files changed, 34 insertions, 23 deletions

diff --git a/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp b/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
index 84ce40589a3..b53338f4d0b 100644
--- a/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
@@ -49,7 +49,7 @@ struct ControllerFixtureBase : public ::testing::Test
     std::shared_ptr<BucketMoveJob>  _bmj;
     MyCountJobRunner                _runner;
     ControllerFixtureBase(const BlockableMaintenanceJobConfig &blockableConfig, bool storeMoveDoneContexts);
-    ~ControllerFixtureBase();
+    ~ControllerFixtureBase() override;
     ControllerFixtureBase &addReady(const BucketId &bucket) {
         _calc->addReady(bucket);
         return *this;
@@ -142,7 +142,7 @@ const BlockableMaintenanceJobConfig BLOCKABLE_CONFIG(RESOURCE_LIMIT_FACTOR, MAX_
 
 struct ControllerFixture : public ControllerFixtureBase
 {
-    ControllerFixture(const BlockableMaintenanceJobConfig &blockableConfig = BLOCKABLE_CONFIG)
+    explicit ControllerFixture(const BlockableMaintenanceJobConfig &blockableConfig = BLOCKABLE_CONFIG)
         : ControllerFixtureBase(blockableConfig, blockableConfig.getMaxOutstandingMoveOps() != MAX_OUTSTANDING_OPS)
     {
         _builder.createDocs(1, 1, 4); // 3 docs
@@ -462,7 +462,7 @@ TEST_F(ControllerFixture, inactive_not_ready_bucket_not_moved_to_ready_if_node_i
     EXPECT_EQ(0u, docsMoved().size());
 }
 
-TEST_F(ControllerFixture, explicitly_active_not_ready_bucket_can_be_moved_to_ready_even_if_node_is_marked_as_retired)
+TEST_F(ControllerFixture, explicitly_active_not_ready_bucket_can_not_be_moved_to_ready_if_node_is_marked_as_retired)
 {
     _calc->setNodeRetired(true);
     addReady(_ready.bucket(1));
@@ -475,11 +475,24 @@ TEST_F(ControllerFixture, explicitly_active_not_ready_bucket_can_be_moved_to_rea
         EXPECT_TRUE(_bmj->done());
     });
     sync();
-    ASSERT_EQ(2u, docsMoved().size());
-    assertEqual(_notReady.bucket(3), _notReady.docs(3)[0], 2, 1, docsMoved()[0]);
-    assertEqual(_notReady.bucket(3), _notReady.docs(3)[1], 2, 1, docsMoved()[1]);
-    ASSERT_EQ(1u, bucketsModified().size());
-    EXPECT_EQ(_notReady.bucket(3), bucketsModified()[0]);
+    ASSERT_EQ(0u, docsMoved().size());
+    ASSERT_EQ(0u, bucketsModified().size());
+}
+
+TEST_F(ControllerFixture, explicitly_active_not_ready_bucket_can_not_be_moved_to_ready)
+{
+    addReady(_ready.bucket(1));
+    addReady(_ready.bucket(2));
+    remReady(_notReady.bucket(3));
+    _bmj->recompute();
+    masterExecute([this]() {
+        activateBucket(_notReady.bucket(3));
+        _bmj->scanAndMove(4, 3);
+        EXPECT_TRUE(_bmj->done());
+    });
+    sync();
+    ASSERT_EQ(0u, docsMoved().size());
+    ASSERT_EQ(0u, bucketsModified().size());
 }
 
 TEST_F(ControllerFixture, bucket_change_notification_is_not_lost_with_concurrent_bucket_movers)
@@ -545,7 +558,7 @@ TEST_F(ControllerFixture, require_that_notifyCreateBucket_causes_bucket_to_be_re
 
 struct ResourceLimitControllerFixture : public ControllerFixture
 {
-    ResourceLimitControllerFixture(double resourceLimitFactor = RESOURCE_LIMIT_FACTOR) :
+    explicit ResourceLimitControllerFixture(double resourceLimitFactor = RESOURCE_LIMIT_FACTOR) :
         ControllerFixture(BlockableMaintenanceJobConfig(resourceLimitFactor, MAX_OUTSTANDING_OPS))
     {}
 
@@ -619,7 +632,7 @@ TEST_F(ResourceLimitControllerFixture_1_2, require_that_bucket_move_uses_resourc
 
 struct MaxOutstandingMoveOpsFixture : public ControllerFixtureBase
 {
-    MaxOutstandingMoveOpsFixture(uint32_t maxOutstandingOps)
+    explicit MaxOutstandingMoveOpsFixture(uint32_t maxOutstandingOps)
         : ControllerFixtureBase(BlockableMaintenanceJobConfig(RESOURCE_LIMIT_FACTOR, maxOutstandingOps), true)
     {
         _builder.createDocs(1, 1, 2);
diff --git a/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.h b/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.h
index 12c205e8011..a32f3c06513 100644
--- a/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.h
+++ b/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.h
@@ -36,12 +36,12 @@ namespace bucketdb { class IBucketCreateNotifier; }
  * 3 - Actual movement is then done in master thread while still holding bucket lock. Once bucket has fully moved
  *     bucket modified notification is sent.
  */
-class BucketMoveJob : public BlockableMaintenanceJob,
-                      public IClusterStateChangedHandler,
-                      public bucketdb::IBucketCreateListener,
-                      public IBucketStateChangedHandler,
-                      public IDiskMemUsageListener,
-                      public std::enable_shared_from_this<BucketMoveJob>
+class BucketMoveJob final : public BlockableMaintenanceJob,
+                            public IClusterStateChangedHandler,
+                            public bucketdb::IBucketCreateListener,
+                            public IBucketStateChangedHandler,
+                            public IDiskMemUsageListener,
+                            public std::enable_shared_from_this<BucketMoveJob>
 {
 private:
     using BucketExecutor = storage::spi::BucketExecutor;
diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.h b/searchcore/src/vespa/searchcore/proton/server/documentdb.h
index 39efdd11619..9a37af71bab 100644
--- a/searchcore/src/vespa/searchcore/proton/server/documentdb.h
+++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.h
@@ -377,7 +377,7 @@ public:
     /**
      * Reference counting
      */
-    vespalib::RetainGuard retain() { return vespalib::RetainGuard(_refCount); }
+    vespalib::RetainGuard retain() { return {_refCount}; }
 
     bool getDelayedConfig() const { return _state.getDelayedConfig(); }
     void replayConfig(SerialNum serialNum) override;
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
index e5498a62836..f48214e18f8 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
@@ -643,7 +643,7 @@ Proton::addDocumentDB(const document::DocumentType &docType,
     } catch (vespalib::Exception &e) {
         LOG(warning, "Failed to start database for document type '%s'; %s",
             docTypeName.toString().c_str(), e.what());
-        return DocumentDB::SP();
+        return {};
     }
     // Wait for replay done on document dbs added due to reconfigs, since engines are already up and running.
     // Also wait for document db reaching online state if initializing in sequence.
@@ -940,11 +940,10 @@ struct DocumentDBMapExplorer : vespalib::StateExplorer {
         return names;
     }
     std::unique_ptr<vespalib::StateExplorer> get_child(vespalib::stringref name) const override {
-        typedef std::unique_ptr<StateExplorer> Explorer_UP;
         std::shared_lock<std::shared_mutex> guard(mutex);
         auto result = documentDBMap.find(DocTypeName(vespalib::string(name)));
         if (result == documentDBMap.end()) {
-            return Explorer_UP(nullptr);
+            return {};
         }
         return std::make_unique<DocumentDBExplorer>(result->second);
     }
@@ -966,7 +965,6 @@ Proton::get_children_names() const
 std::unique_ptr<vespalib::StateExplorer>
 Proton::get_child(vespalib::stringref name) const
 {
-    typedef std::unique_ptr<StateExplorer> Explorer_UP;
     if (name == MATCH_ENGINE && _matchEngine) {
         return std::make_unique<StateExplorerProxy>(*_matchEngine);
     } else if (name == DOCUMENT_DB) {
@@ -990,7 +988,7 @@ Proton::get_child(vespalib::stringref name) const
     } else if (name == HW_INFO) {
         return std::make_unique<HwInfoExplorer>(_hw_info);
     }
-    return Explorer_UP(nullptr);
+    return {};
 }
 
 std::shared_ptr<IDocumentDBReferenceRegistry>
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.h b/searchcore/src/vespa/searchcore/proton/server/proton.h
index e97764ae32c..0d4b067eea1 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.h
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.h
@@ -76,7 +76,7 @@ private:
         pid_t _pid;
 
     public:
-        ProtonFileHeaderContext(const vespalib::string &creator);
+        explicit ProtonFileHeaderContext(const vespalib::string &creator);
         ~ProtonFileHeaderContext() override;
 
         void addTags(vespalib::GenericHeader &header, const vespalib::string &name) const override;