From cff43ac7d5a296db8f49eb7277435947b9662339 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 9 Apr 2021 18:16:26 +0000 Subject: Make the scope of the lidspace handler fit its lifetime. --- .../maintenancecontroller_test.cpp | 29 ++++------------------ .../vespa/searchcore/proton/server/documentdb.cpp | 11 ++------ .../vespa/searchcore/proton/server/documentdb.h | 2 -- .../proton/server/maintenance_jobs_injector.cpp | 12 ++++++--- .../proton/server/maintenance_jobs_injector.h | 1 - 5 files changed, 15 insertions(+), 40 deletions(-) (limited to 'searchcore') diff --git a/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp b/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp index ff1b29d646e..b5e2e9d0b01 100644 --- a/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp +++ b/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp @@ -349,22 +349,6 @@ struct MyLongRunningJob : public BlockableMaintenanceJob using MyAttributeManager = test::MockAttributeManager; -struct MockLidSpaceCompactionHandler : public ILidSpaceCompactionHandler -{ - vespalib::string name; - - explicit MockLidSpaceCompactionHandler(const vespalib::string &name_) : name(name_) {} - vespalib::string getName() const override { return name; } - void set_operation_listener(documentmetastore::OperationListener::SP) override {} - uint32_t getSubDbId() const override { return 0; } - search::LidUsageStats getLidStatus() const override { return search::LidUsageStats(); } - IDocumentScanIterator::UP getIterator() const override { return IDocumentScanIterator::UP(); } - MoveOperation::UP createMoveOperation(const search::DocumentMetaData &, uint32_t) const override { return MoveOperation::UP(); } - void handleMove(const MoveOperation &, IDestructorCallback::SP) override {} - void handleCompactLidSpace(const CompactLidSpaceOperation &, std::shared_ptr) override {} - search::DocumentMetaData getMetaData(uint32_t ) const override { return {}; } -}; - class MaintenanceControllerFixture { public: @@ -384,7 +368,6 @@ public: MyDocumentSubDB _notReady; MySessionCachePruner _gsp; MyFeedHandler _fh; - ILidSpaceCompactionHandler::Vector _lscHandlers; DocumentDBMaintenanceConfig::SP _mcCfg; bool _injectDefaultJobs; DocumentDBJobTrackers _jobTrackers; @@ -804,7 +787,6 @@ MaintenanceControllerFixture::MaintenanceControllerFixture() _notReady(2u, SubDbType::NOTREADY, _builder.getRepo(), _bucketDB, _docTypeName), _gsp(), _fh(_executor._threadId), - _lscHandlers(), _mcCfg(new DocumentDBMaintenanceConfig), _injectDefaultJobs(true), _jobTrackers(), @@ -864,7 +846,7 @@ void MaintenanceControllerFixture::injectMaintenanceJobs() { if (_injectDefaultJobs) { - MaintenanceJobsInjector::injectJobs(_mc, *_mcCfg, _bucketExecutor, _fh, _gsp, _lscHandlers, _fh, _mc, + MaintenanceJobsInjector::injectJobs(_mc, *_mcCfg, _bucketExecutor, _fh, _gsp, _fh, _mc, _bucketCreateNotifier, _docTypeName.getName(), makeBucketSpace(), _fh, _fh, _bmc, _clusterStateHandler, _bucketHandler, _calc, _diskMemUsageNotifier, _jobTrackers, _readyAttributeManager, _notReadyAttributeManager, @@ -1288,18 +1270,17 @@ containsJobAndExecutedBy(const MaintenanceController::JobList &jobs, const vespa TEST_F("require that lid space compaction jobs can be disabled", MaintenanceControllerFixture) { - f._lscHandlers.push_back(std::make_unique("my_handler")); f.forwardMaintenanceConfig(); { auto jobs = f._mc.getJobList(); - EXPECT_EQUAL(6u, jobs.size()); - EXPECT_TRUE(containsJob(jobs, "lid_space_compaction.my_handler")); + EXPECT_EQUAL(8u, jobs.size()); + EXPECT_TRUE(containsJob(jobs, "lid_space_compaction.searchdocument.my_sub_db")); } f.setLidSpaceCompactionConfig(DocumentDBLidSpaceCompactionConfig::createDisabled()); { auto jobs = f._mc.getJobList(); EXPECT_EQUAL(5u, jobs.size()); - EXPECT_FALSE(containsJob(jobs, "lid_space_compaction.my_handler")); + EXPECT_FALSE(containsJob(jobs, "lid_space_compaction.searchdocument.my_sub_db")); } } @@ -1307,7 +1288,7 @@ TEST_F("require that maintenance jobs are run by correct executor", MaintenanceC { f.injectMaintenanceJobs(); auto jobs = f._mc.getJobList(); - EXPECT_EQUAL(5u, jobs.size()); + EXPECT_EQUAL(8u, jobs.size()); EXPECT_TRUE(containsJobAndExecutedBy(jobs, "heart_beat", f._threadService)); EXPECT_TRUE(containsJobAndExecutedBy(jobs, "prune_session_cache", f._genericExecutor)); EXPECT_TRUE(containsJobAndExecutedBy(jobs, "prune_removed_documents.searchdocument", f._threadService)); diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp index 3cb5c31e13c..d99e579b89f 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp @@ -9,7 +9,6 @@ #include "feedhandler.h" #include "idocumentdbowner.h" #include "idocumentsubdb.h" -#include "lid_space_compaction_handler.h" #include "maintenance_jobs_injector.h" #include "reconfig_params.h" #include @@ -174,7 +173,6 @@ DocumentDB::DocumentDB(const vespalib::string &baseDir, DocumentSubDBCollection::Config(protonCfg.numsearcherthreads), hwInfo), _maintenanceController(_writeService.master(), sharedExecutor, _docTypeName), - _lidSpaceCompactionHandlers(), _jobTrackers(), _calc(), _metricsUpdater(_subDBs, _writeService, _jobTrackers, *_sessionManager, _writeFilter) @@ -927,16 +925,11 @@ DocumentDB::injectMaintenanceJobs(const DocumentDBMaintenanceConfig &config, std { // Called by executor thread _maintenanceController.killJobs(); - _lidSpaceCompactionHandlers.clear(); - _lidSpaceCompactionHandlers.push_back(std::make_shared(_maintenanceController.getReadySubDB(), _docTypeName.getName())); - _lidSpaceCompactionHandlers.push_back(std::make_shared(_maintenanceController.getRemSubDB(), _docTypeName.getName())); - _lidSpaceCompactionHandlers.push_back(std::make_shared(_maintenanceController.getNotReadySubDB(), _docTypeName.getName())); MaintenanceJobsInjector::injectJobs(_maintenanceController, config, _bucketExecutor, *_feedHandler, // IHeartBeatHandler *_sessionManager, // ISessionCachePruner - _lidSpaceCompactionHandlers, *_feedHandler, // IOperationStorer _maintenanceController, // IFrozenBucketHandler _subDBs.getBucketCreateNotifier(), @@ -1026,8 +1019,8 @@ namespace { void notifyBucketsChanged(const documentmetastore::IBucketHandler &metaStore, - IBucketModifiedHandler &handler, - const vespalib::string &name) + IBucketModifiedHandler &handler, + const vespalib::string &name) { bucketdb::Guard buckets = metaStore.getBucketDB().takeGuard(); for (const auto &kv : *buckets) { diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.h b/searchcore/src/vespa/searchcore/proton/server/documentdb.h index d8bba30e650..dd3d821e291 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdb.h +++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.h @@ -13,7 +13,6 @@ #include "executorthreadingservice.h" #include "i_document_subdb_owner.h" #include "i_feed_handler_owner.h" -#include "i_lid_space_compaction_handler.h" #include "ifeedview.h" #include "ireplayconfig.h" #include "maintenancecontroller.h" @@ -123,7 +122,6 @@ private: std::unique_ptr _feedHandler; DocumentSubDBCollection _subDBs; MaintenanceController _maintenanceController; - ILidSpaceCompactionHandler::Vector _lidSpaceCompactionHandlers; DocumentDBJobTrackers _jobTrackers; std::shared_ptr _calc; DocumentDBMetricsUpdater _metricsUpdater; diff --git a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp index bd6c09934be..3d8348f3591 100644 --- a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp @@ -6,6 +6,7 @@ #include "job_tracked_maintenance_job.h" #include "lid_space_compaction_job.h" #include "lid_space_compaction_job_take2.h" +#include "lid_space_compaction_handler.h" #include "maintenance_jobs_injector.h" #include "prune_session_cache_job.h" #include "pruneremoveddocumentsjob.h" @@ -28,7 +29,7 @@ void injectLidSpaceCompactionJobs(MaintenanceController &controller, const DocumentDBMaintenanceConfig &config, storage::spi::BucketExecutor & bucketExecutor, - const ILidSpaceCompactionHandler::Vector &lscHandlers, + ILidSpaceCompactionHandler::Vector lscHandlers, IOperationStorer &opStorer, IFrozenBucketHandler &fbHandler, const IJobTracker::SP &tracker, @@ -117,7 +118,6 @@ MaintenanceJobsInjector::injectJobs(MaintenanceController &controller, storage::spi::BucketExecutor & bucketExecutor, IHeartBeatHandler &hbHandler, matching::ISessionCachePruner &scPruner, - const ILidSpaceCompactionHandler::Vector &lscHandlers, IOperationStorer &opStorer, IFrozenBucketHandler &fbHandler, bucketdb::IBucketCreateNotifier &bucketCreateNotifier, @@ -146,8 +146,12 @@ MaintenanceJobsInjector::injectJobs(MaintenanceController &controller, controller.registerJobInMasterThread(trackJob(jobTrackers.getRemovedDocumentsPrune(), std::move(pruneRDjob))); if (!config.getLidSpaceCompactionConfig().isDisabled()) { - injectLidSpaceCompactionJobs(controller, config, bucketExecutor, lscHandlers, opStorer, fbHandler, - jobTrackers.getLidSpaceCompact(), diskMemUsageNotifier, + ILidSpaceCompactionHandler::Vector lidSpaceCompactionHandlers; + lidSpaceCompactionHandlers.push_back(std::make_shared(controller.getReadySubDB(), docTypeName)); + lidSpaceCompactionHandlers.push_back(std::make_shared(controller.getRemSubDB(), docTypeName)); + lidSpaceCompactionHandlers.push_back(std::make_shared(controller.getNotReadySubDB(), docTypeName)); + injectLidSpaceCompactionJobs(controller, config, bucketExecutor, std::move(lidSpaceCompactionHandlers), + opStorer, fbHandler, jobTrackers.getLidSpaceCompact(), diskMemUsageNotifier, clusterStateChangedNotifier, calc, bucketSpace); } diff --git a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.h b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.h index 86049736a38..563ef227fcf 100644 --- a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.h +++ b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.h @@ -37,7 +37,6 @@ struct MaintenanceJobsInjector storage::spi::BucketExecutor & bucketExecutor, IHeartBeatHandler &hbHandler, matching::ISessionCachePruner &scPruner, - const ILidSpaceCompactionHandler::Vector &lscHandlers, IOperationStorer &opStorer, IFrozenBucketHandler &fbHandler, bucketdb::IBucketCreateNotifier &bucketCreateNotifier, -- cgit v1.2.3 From 942bd7bafc0624cf96dc709edb019eacdc5f988f Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 9 Apr 2021 20:37:21 +0000 Subject: Keep shared_ptrs to BucketMove og LidSpace jobs. --- .../searchcore/proton/server/bucketmovejobv2.cpp | 1 + .../vespa/searchcore/proton/server/bucketmovejobv2.h | 3 ++- .../searchcore/proton/server/i_maintenance_job.h | 3 ++- .../proton/server/job_tracked_maintenance_job.cpp | 6 +++--- .../proton/server/job_tracked_maintenance_job.h | 4 ++-- .../proton/server/lid_space_compaction_job_take2.cpp | 1 + .../proton/server/lid_space_compaction_job_take2.h | 2 +- .../proton/server/maintenance_jobs_injector.cpp | 20 ++++++++++---------- 8 files changed, 22 insertions(+), 18 deletions(-) (limited to 'searchcore') diff --git a/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.cpp b/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.cpp index 24a4d6d5dee..fc2eeaab661 100644 --- a/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.cpp @@ -73,6 +73,7 @@ BucketMoveJobV2::BucketMoveJobV2(const std::shared_ptr & bucketdb::IBucketCreateListener(), IBucketStateChangedHandler(), IDiskMemUsageListener(), + std::enable_shared_from_this(), _calc(calc), _moveHandler(moveHandler), _modifiedHandler(modifiedHandler), diff --git a/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.h b/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.h index 620b76ac81c..eba8fd9a62c 100644 --- a/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.h +++ b/searchcore/src/vespa/searchcore/proton/server/bucketmovejobv2.h @@ -37,7 +37,8 @@ class BucketMoveJobV2 : public BlockableMaintenanceJob, public IClusterStateChangedHandler, public bucketdb::IBucketCreateListener, public IBucketStateChangedHandler, - public IDiskMemUsageListener + public IDiskMemUsageListener, + public std::enable_shared_from_this { private: using BucketExecutor = storage::spi::BucketExecutor; diff --git a/searchcore/src/vespa/searchcore/proton/server/i_maintenance_job.h b/searchcore/src/vespa/searchcore/proton/server/i_maintenance_job.h index d0175d69e71..3972072b41d 100644 --- a/searchcore/src/vespa/searchcore/proton/server/i_maintenance_job.h +++ b/searchcore/src/vespa/searchcore/proton/server/i_maintenance_job.h @@ -23,7 +23,8 @@ private: const vespalib::duration _interval; public: - typedef std::unique_ptr UP; + using UP = std::unique_ptr; + using SP = std::shared_ptr; IMaintenanceJob(const vespalib::string &name, vespalib::duration delay, diff --git a/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.cpp b/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.cpp index 59324cc9fe2..aa078376ece 100644 --- a/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.cpp @@ -4,10 +4,10 @@ namespace proton { -JobTrackedMaintenanceJob::JobTrackedMaintenanceJob(const IJobTracker::SP &tracker, - IMaintenanceJob::UP job) +JobTrackedMaintenanceJob::JobTrackedMaintenanceJob(IJobTracker::SP tracker, + IMaintenanceJob::SP job) : IMaintenanceJob(job->getName(), job->getDelay(), job->getInterval()), - _tracker(tracker), + _tracker(std::move(tracker)), _job(std::move(job)), _running(false) { diff --git a/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.h b/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.h index 9acdee557fd..0e1b2b00ce5 100644 --- a/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.h +++ b/searchcore/src/vespa/searchcore/proton/server/job_tracked_maintenance_job.h @@ -13,11 +13,11 @@ class JobTrackedMaintenanceJob : public IMaintenanceJob { private: IJobTracker::SP _tracker; - IMaintenanceJob::UP _job; + IMaintenanceJob::SP _job; bool _running; public: - JobTrackedMaintenanceJob(const IJobTracker::SP &tracker, IMaintenanceJob::UP job); + JobTrackedMaintenanceJob(IJobTracker::SP tracker, IMaintenanceJob::SP job); ~JobTrackedMaintenanceJob() override; bool isBlocked() const override { return _job->isBlocked(); } diff --git a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.cpp b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.cpp index 042cd70f7e0..70a63a4a4eb 100644 --- a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.cpp @@ -121,6 +121,7 @@ CompactionJob::CompactionJob(const DocumentDBLidSpaceCompactionConfig &config, document::BucketSpace bucketSpace) : LidSpaceCompactionJobBase(config, std::move(handler), opStorer, diskMemUsageNotifier, blockableConfig, clusterStateChangedNotifier, nodeRetired), + std::enable_shared_from_this(), _master(master), _bucketExecutor(bucketExecutor), _bucketSpace(bucketSpace), diff --git a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.h b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.h index b73f971f8ee..21445de8d85 100644 --- a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.h +++ b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_take2.h @@ -21,7 +21,7 @@ namespace proton::lidspace { * Moves documents from higher lids to lower lids. It uses a BucketExecutor that ensures that the bucket * is locked for changes while the document is moved. */ -class CompactionJob : public LidSpaceCompactionJobBase +class CompactionJob : public LidSpaceCompactionJobBase, public std::enable_shared_from_this { private: using BucketExecutor = storage::spi::BucketExecutor; diff --git a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp index 3d8348f3591..e31adc45888 100644 --- a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp @@ -20,9 +20,9 @@ namespace proton { namespace { IMaintenanceJob::UP -trackJob(const IJobTracker::SP &tracker, IMaintenanceJob::UP job) +trackJob(IJobTracker::SP tracker, std::shared_ptr job) { - return std::make_unique(tracker, std::move(job)); + return std::make_unique(std::move(tracker), std::move(job)); } void @@ -32,16 +32,16 @@ injectLidSpaceCompactionJobs(MaintenanceController &controller, ILidSpaceCompactionHandler::Vector lscHandlers, IOperationStorer &opStorer, IFrozenBucketHandler &fbHandler, - const IJobTracker::SP &tracker, + IJobTracker::SP tracker, IDiskMemUsageNotifier &diskMemUsageNotifier, IClusterStateChangedNotifier &clusterStateChangedNotifier, const std::shared_ptr &calc, document::BucketSpace bucketSpace) { for (auto &lidHandler : lscHandlers) { - std::unique_ptr job; + std::shared_ptr job; if (config.getLidSpaceCompactionConfig().useBucketExecutor()) { - job = std::make_unique( + job = std::make_shared( config.getLidSpaceCompactionConfig(), std::move(lidHandler), opStorer, controller.masterThread(), bucketExecutor, diskMemUsageNotifier, @@ -50,7 +50,7 @@ injectLidSpaceCompactionJobs(MaintenanceController &controller, (calc ? calc->nodeRetired() : false), bucketSpace); } else { - job = std::make_unique( + job = std::make_shared( config.getLidSpaceCompactionConfig(), std::move(lidHandler), opStorer, fbHandler, diskMemUsageNotifier, @@ -58,7 +58,7 @@ injectLidSpaceCompactionJobs(MaintenanceController &controller, clusterStateChangedNotifier, (calc ? calc->nodeRetired() : false)); } - controller.registerJobInMasterThread(trackJob(tracker, std::move(job))); + controller.registerJobInMasterThread(trackJob(std::move(tracker), std::move(job))); } } @@ -78,9 +78,9 @@ injectBucketMoveJob(MaintenanceController &controller, DocumentDBJobTrackers &jobTrackers, IDiskMemUsageNotifier &diskMemUsageNotifier) { - std::unique_ptr bmj; + std::shared_ptr bmj; if (config.getBucketMoveConfig().useBucketExecutor()) { - bmj = std::make_unique(calc, + bmj = std::make_shared(calc, moveHandler, bucketModifiedHandler, controller.masterThread(), @@ -94,7 +94,7 @@ injectBucketMoveJob(MaintenanceController &controller, config.getBlockableJobConfig(), docTypeName, bucketSpace); } else { - bmj = std::make_unique(calc, + bmj = std::make_shared(calc, moveHandler, bucketModifiedHandler, controller.getReadySubDB(), -- cgit v1.2.3 From 0560d261aa04f1caed107ef82c026d3dad9f5c45 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Fri, 9 Apr 2021 21:54:45 +0000 Subject: Do not std::move shared_ptr in loop. --- .../src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'searchcore') diff --git a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp index e31adc45888..7cf4412f533 100644 --- a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp @@ -58,7 +58,7 @@ injectLidSpaceCompactionJobs(MaintenanceController &controller, clusterStateChangedNotifier, (calc ? calc->nodeRetired() : false)); } - controller.registerJobInMasterThread(trackJob(std::move(tracker), std::move(job))); + controller.registerJobInMasterThread(trackJob(tracker, std::move(job))); } } -- cgit v1.2.3