diff options
Diffstat (limited to 'searchcore/src')
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; |