Merge pull request #17350 from vespa-engine/balder/correct-the-scope-of-lidspace-handlers

Make the scope of the lidspace handler fit its lifetime.

12 files changed, 36 insertions, 57 deletions

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<IDestructorCallback>) 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<MockLidSpaceCompactionHandler>("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/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<IBucketStateCalculator> &
       bucketdb::IBucketCreateListener(),
       IBucketStateChangedHandler(),
       IDiskMemUsageListener(),
+      std::enable_shared_from_this<BucketMoveJobV2>(),
       _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<BucketMoveJobV2>
 {
 private:
     using BucketExecutor = storage::spi::BucketExecutor;
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 <vespa/document/repo/documenttyperepo.h>
@@ -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<LidSpaceCompactionHandler>(_maintenanceController.getReadySubDB(), _docTypeName.getName()));
-    _lidSpaceCompactionHandlers.push_back(std::make_shared<LidSpaceCompactionHandler>(_maintenanceController.getRemSubDB(), _docTypeName.getName()));
-    _lidSpaceCompactionHandlers.push_back(std::make_shared<LidSpaceCompactionHandler>(_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>                  _feedHandler;
     DocumentSubDBCollection                       _subDBs;
     MaintenanceController                         _maintenanceController;
-    ILidSpaceCompactionHandler::Vector            _lidSpaceCompactionHandlers;
     DocumentDBJobTrackers                         _jobTrackers;
     std::shared_ptr<IBucketStateCalculator>       _calc;
     DocumentDBMetricsUpdater                      _metricsUpdater;
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<IMaintenanceJob> UP;
+    using UP = std::unique_ptr<IMaintenanceJob>;
+    using SP = std::shared_ptr<IMaintenanceJob>;
 
     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<CompactionJob>(),
       _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<CompactionJob>
 {
 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 bd6c09934be..7cf4412f533 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"
@@ -19,28 +20,28 @@ namespace proton {
 namespace {
 
 IMaintenanceJob::UP
-trackJob(const IJobTracker::SP &tracker, IMaintenanceJob::UP job)
+trackJob(IJobTracker::SP tracker, std::shared_ptr<IMaintenanceJob> job)
 {
-    return std::make_unique<JobTrackedMaintenanceJob>(tracker, std::move(job));
+    return std::make_unique<JobTrackedMaintenanceJob>(std::move(tracker), std::move(job));
 }
 
 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,
+                             IJobTracker::SP tracker,
                              IDiskMemUsageNotifier &diskMemUsageNotifier,
                              IClusterStateChangedNotifier &clusterStateChangedNotifier,
                              const std::shared_ptr<IBucketStateCalculator> &calc,
                              document::BucketSpace bucketSpace)
 {
     for (auto &lidHandler : lscHandlers) {
-        std::unique_ptr<IMaintenanceJob> job;
+        std::shared_ptr<IMaintenanceJob> job;
         if (config.getLidSpaceCompactionConfig().useBucketExecutor()) {
-            job = std::make_unique<lidspace::CompactionJob>(
+            job = std::make_shared<lidspace::CompactionJob>(
                     config.getLidSpaceCompactionConfig(),
                     std::move(lidHandler), opStorer, controller.masterThread(), bucketExecutor,
                     diskMemUsageNotifier,
@@ -49,7 +50,7 @@ injectLidSpaceCompactionJobs(MaintenanceController &controller,
                     (calc ? calc->nodeRetired() : false),
                     bucketSpace);
         } else {
-            job = std::make_unique<LidSpaceCompactionJob>(
+            job = std::make_shared<LidSpaceCompactionJob>(
                     config.getLidSpaceCompactionConfig(),
                     std::move(lidHandler), opStorer, fbHandler,
                     diskMemUsageNotifier,
@@ -77,9 +78,9 @@ injectBucketMoveJob(MaintenanceController &controller,
                     DocumentDBJobTrackers &jobTrackers,
                     IDiskMemUsageNotifier &diskMemUsageNotifier)
 {
-    std::unique_ptr<IMaintenanceJob> bmj;
+    std::shared_ptr<IMaintenanceJob> bmj;
     if (config.getBucketMoveConfig().useBucketExecutor()) {
-        bmj = std::make_unique<BucketMoveJobV2>(calc,
+        bmj = std::make_shared<BucketMoveJobV2>(calc,
                                                 moveHandler,
                                                 bucketModifiedHandler,
                                                 controller.masterThread(),
@@ -93,7 +94,7 @@ injectBucketMoveJob(MaintenanceController &controller,
                                                 config.getBlockableJobConfig(),
                                                 docTypeName, bucketSpace);
     } else {
-        bmj = std::make_unique<BucketMoveJob>(calc,
+        bmj = std::make_shared<BucketMoveJob>(calc,
                                               moveHandler,
                                               bucketModifiedHandler,
                                               controller.getReadySubDB(),
@@ -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<LidSpaceCompactionHandler>(controller.getReadySubDB(), docTypeName));
+        lidSpaceCompactionHandlers.push_back(std::make_shared<LidSpaceCompactionHandler>(controller.getRemSubDB(), docTypeName));
+        lidSpaceCompactionHandlers.push_back(std::make_shared<LidSpaceCompactionHandler>(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,