Merge pull request #24057 from vespa-engine/balder/only-consider-clusterstate-when-making-bucket-ready

Balder/only consider clusterstate when making bucket ready

6 files changed, 46 insertions, 37 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..44075de4179 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
@@ -433,9 +433,9 @@ TEST_F(ControllerFixture, require_that_de_activated_bucket_is_not_moved_if_new_c
 
 TEST_F(ControllerFixture, ready_bucket_not_moved_to_not_ready_if_node_is_marked_as_retired)
 {
-    _calc->setNodeRetired(true);
     // Bucket 2 would be moved from ready to not ready in a non-retired case, but not when retired.
-    addReady(_ready.bucket(1));
+    remReady(_ready.bucket(1));
+    _calc->setNodeRetired(true);
     masterExecute([this]() {
         _bmj->recompute();
         _bmj->scanAndMove(4, 3);
@@ -449,10 +449,10 @@ TEST_F(ControllerFixture, ready_bucket_not_moved_to_not_ready_if_node_is_marked_
 // but test this case for the sake of completion.
 TEST_F(ControllerFixture, inactive_not_ready_bucket_not_moved_to_ready_if_node_is_marked_as_retired)
 {
+    remReady(_ready.bucket(1));
+    remReady(_ready.bucket(2));
+    remReady(_notReady.bucket(3));
     _calc->setNodeRetired(true);
-    addReady(_ready.bucket(1));
-    addReady(_ready.bucket(2));
-    addReady(_notReady.bucket(3));
     masterExecute([this]() {
         _bmj->recompute();
         _bmj->scanAndMove(4, 3);
@@ -462,12 +462,28 @@ 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)
 {
+    remReady(_ready.bucket(1));
+    remReady(_ready.bucket(2));
+    remReady(_notReady.bucket(3));
     _calc->setNodeRetired(true);
+    _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, explicitly_active_not_ready_bucket_can_not_be_moved_to_ready)
+{
     addReady(_ready.bucket(1));
     addReady(_ready.bucket(2));
-    addReady(_notReady.bucket(3));
+    remReady(_notReady.bucket(3));
     _bmj->recompute();
     masterExecute([this]() {
         activateBucket(_notReady.bucket(3));
@@ -475,11 +491,8 @@ 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, 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.cpp b/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.cpp
index 8be55433fdd..634eac19923 100644
--- a/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.cpp
@@ -131,14 +131,13 @@ BucketMoveJob::create(std::shared_ptr<IBucketStateCalculator> calc,
                       const vespalib::string &docTypeName,
                       document::BucketSpace bucketSpace)
 {
-    return std::shared_ptr<BucketMoveJob>(
-            new BucketMoveJob(std::move(calc), std::move(dbRetainer), moveHandler, modifiedHandler, master, bucketExecutor, ready, notReady,
+    return {new BucketMoveJob(std::move(calc), std::move(dbRetainer), moveHandler, modifiedHandler, master, bucketExecutor, ready, notReady,
                               bucketCreateNotifier, clusterStateChangedNotifier, bucketStateChangedNotifier,
                               diskMemUsageNotifier, blockableConfig, docTypeName, bucketSpace),
             [&master](auto job) {
                 auto failed = master.execute(makeLambdaTask([job]() { delete job; }));
                 assert(!failed);
-            });
+            }};
 }
 
 BucketMoveJob::NeedResult
@@ -149,17 +148,16 @@ BucketMoveJob::needMove(BucketId bucketId, const BucketStateWrapper &itr) const
     if (!hasReadyDocs && !hasNotReadyDocs) {
         return noMove; // No documents for bucket in ready or notready subdbs
     }
-    const bool isActive = itr.isActive();
     // No point in moving buckets when node is retired and everything will be deleted soon.
-    // However, allow moving of explicitly activated buckets, as this implies a lack of other good replicas.
-    if (!_calc || (_calc->nodeRetired() && !isActive)) {
+    if (!_calc || _calc->nodeRetired()) {
         return noMove;
     }
     const Trinary shouldBeReady = _calc->shouldBeReady(document::Bucket(_bucketSpace, bucketId));
     if (shouldBeReady == Trinary::Undefined) {
         return noMove;
     }
-    const bool wantReady = (shouldBeReady == Trinary::True) || isActive;
+    const bool isActive = itr.isActive();
+    const bool wantReady = (shouldBeReady == Trinary::True);
     LOG(spam, "needMove(): bucket(%s), shouldBeReady(%s), active(%s)",
         bucketId.toString().c_str(), toStr(shouldBeReady), toStr(isActive));
     if (wantReady) {
@@ -168,7 +166,7 @@ BucketMoveJob::needMove(BucketId bucketId, const BucketStateWrapper &itr) const
         }
     } else {
         if (isActive) {
-            return noMove; // Do not move rom ready to not ready when active
+            return noMove; // Do not move from ready to not ready when active
         }
         if (!hasReadyDocs) {
             return noMove; // No ready bucket to make notready
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;