Merge pull request #25237 from vespa-engine/balder/gc-unused-executor-magic

There is only one the master executor available to the MaintenanceJobs.

5 files changed, 14 insertions, 55 deletions

diff --git a/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp b/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp
index 47555288a10..debad7eaf26 100644
--- a/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/maintenancecontroller/maintenancecontroller_test.cpp
@@ -917,7 +917,7 @@ TEST_F("require that a simple maintenance job is executed", MaintenanceControlle
 {
     auto job = std::make_unique<MySimpleJob>(200ms, 200ms, 3);
     MySimpleJob &myJob = *job;
-    f._mc.registerJobInMasterThread(std::move(job));
+    f._mc.registerJob(std::move(job));
     f._injectDefaultJobs = false;
     f.startMaintenance();
     bool done = myJob._latch.await(TIMEOUT);
@@ -929,7 +929,7 @@ TEST_F("require that a split maintenance job is executed", MaintenanceController
 {
     auto job = std::make_unique<MySplitJob>(200ms, TIMEOUT * 2, 3);
     MySplitJob &myJob = *job;
-    f._mc.registerJobInMasterThread(std::move(job));
+    f._mc.registerJob(std::move(job));
     f._injectDefaultJobs = false;
     f.startMaintenance();
     bool done = myJob._latch.await(TIMEOUT);
@@ -942,7 +942,7 @@ TEST_F("require that blocked jobs are not executed", MaintenanceControllerFixtur
     auto job = std::make_unique<MySimpleJob>(200ms, 200ms, 0);
     MySimpleJob &myJob = *job;
     myJob.block();
-    f._mc.registerJobInMasterThread(std::move(job));
+    f._mc.registerJob(std::move(job));
     f._injectDefaultJobs = false;
     f.startMaintenance();
     std::this_thread::sleep_for(2s);
@@ -955,8 +955,8 @@ TEST_F("require that maintenance controller state list jobs", MaintenanceControl
         auto job1 = std::make_unique<MySimpleJob>(TIMEOUT * 2, TIMEOUT * 2, 0);
         auto job2 = std::make_unique<MyLongRunningJob>(200ms, 200ms);
         auto &longRunningJob = dynamic_cast<MyLongRunningJob &>(*job2);
-        f._mc.registerJobInMasterThread(std::move(job1));
-        f._mc.registerJobInMasterThread(std::move(job2));
+        f._mc.registerJob(std::move(job1));
+        f._mc.registerJob(std::move(job2));
         f._injectDefaultJobs = false;
         f.startMaintenance();
         longRunningJob._firstRun.await(TIMEOUT);
@@ -994,14 +994,6 @@ containsJob(const MaintenanceController::JobList &jobs, const vespalib::string &
     return findJob(jobs, jobName) != nullptr;
 }
 
-bool
-containsJobAndExecutedBy(const MaintenanceController::JobList &jobs, const vespalib::string &jobName,
-                         const vespalib::Executor & executor)
-{
-    const auto *job = findJob(jobs, jobName);
-    return (job != nullptr) && (&job->getExecutor() == &executor);
-}
-
 TEST_F("require that lid space compaction jobs can be disabled", MaintenanceControllerFixture)
 {
     f.forwardMaintenanceConfig();
@@ -1018,17 +1010,6 @@ TEST_F("require that lid space compaction jobs can be disabled", MaintenanceCont
     }
 }
 
-TEST_F("require that maintenance jobs are run by correct executor", MaintenanceControllerFixture)
-{
-    f.injectMaintenanceJobs();
-    auto jobs = f._mc.getJobList();
-    EXPECT_EQUAL(7u, jobs.size());
-    EXPECT_TRUE(containsJobAndExecutedBy(jobs, "heart_beat", f._threadService));
-    EXPECT_TRUE(containsJobAndExecutedBy(jobs, "prune_removed_documents.searchdocument", f._threadService));
-    EXPECT_TRUE(containsJobAndExecutedBy(jobs, "move_buckets.searchdocument", f._threadService));
-    EXPECT_TRUE(containsJobAndExecutedBy(jobs, "sample_attribute_usage.searchdocument", f._threadService));
-}
-
 void
 assertPruneRemovedDocumentsConfig(vespalib::duration expDelay, vespalib::duration expInterval, vespalib::duration interval, MaintenanceControllerFixture &f)
 {
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 c188bcc432d..4e2dd1ea86f 100644
--- a/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/maintenance_jobs_injector.cpp
@@ -38,7 +38,7 @@ injectLidSpaceCompactionJobs(MaintenanceController &controller,
                                                    std::move(lidHandler), opStorer, controller.masterThread(),
                                                    bucketExecutor, diskMemUsageNotifier,config.getBlockableJobConfig(),
                                                    clusterStateChangedNotifier, calc && calc->nodeRetired(), bucketSpace);
-        controller.registerJobInMasterThread(trackJob(tracker, std::move(job)));
+        controller.registerJob(trackJob(tracker, std::move(job)));
     }
 }
 
@@ -61,7 +61,7 @@ injectBucketMoveJob(MaintenanceController &controller,
                                      bucketExecutor, controller.getReadySubDB(), controller.getNotReadySubDB(),
                                      bucketCreateNotifier, clusterStateChangedNotifier, bucketStateChangedNotifier,
                                      diskMemUsageNotifier, config.getBlockableJobConfig(), docTypeName, bucketSpace);
-    controller.registerJobInMasterThread(trackJob(jobTrackers.getBucketMove(), std::move(bmj)));
+    controller.registerJob(trackJob(jobTrackers.getBucketMove(), std::move(bmj)));
 }
 
 }
@@ -86,12 +86,12 @@ MaintenanceJobsInjector::injectJobs(MaintenanceController &controller,
                                     IAttributeManagerSP notReadyAttributeManager,
                                     AttributeUsageFilter &attributeUsageFilter)
 {
-    controller.registerJobInMasterThread(std::make_unique<HeartBeatJob>(hbHandler, config.getHeartBeatConfig()));
+    controller.registerJob(std::make_unique<HeartBeatJob>(hbHandler, config.getHeartBeatConfig()));
 
     const auto & docTypeName = controller.getDocTypeName().getName();
     const MaintenanceDocumentSubDB &mRemSubDB(controller.getRemSubDB());
 
-    controller.registerJobInMasterThread(
+    controller.registerJob(
             trackJob(jobTrackers.getRemovedDocumentsPrune(),
                      PruneRemovedDocumentsJob::create(config.getPruneRemovedDocumentsConfig(), controller.retainDB(),
                                                       *mRemSubDB.meta_store(), mRemSubDB.sub_db_id(), bucketSpace,
@@ -113,7 +113,7 @@ MaintenanceJobsInjector::injectJobs(MaintenanceController &controller,
                         moveHandler, bucketModifiedHandler, clusterStateChangedNotifier, bucketStateChangedNotifier,
                         calc, jobTrackers, diskMemUsageNotifier);
 
-    controller.registerJobInMasterThread(
+    controller.registerJob(
             std::make_unique<SampleAttributeUsageJob>(std::move(readyAttributeManager),
                                                       std::move(notReadyAttributeManager),
                                                       attributeUsageFilter, docTypeName,
diff --git a/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.cpp b/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.cpp
index adafa4c6217..c86019005dc 100644
--- a/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.cpp
@@ -30,13 +30,6 @@ public:
     void run() override { _job->run(); }
 };
 
-bool
-isRunnable(const MaintenanceJobRunner & job, const Executor * master) {
-    return (&job.getExecutor() == master)
-           ? false
-           : job.isRunnable();
-}
-
 }
 
 MaintenanceController::MaintenanceController(FNET_Transport & transport,
@@ -61,18 +54,11 @@ MaintenanceController::~MaintenanceController()
 }
 
 void
-MaintenanceController::registerJobInMasterThread(IMaintenanceJob::UP job)
-{
-    // Called by master write thread
-    registerJob(_masterThread, std::move(job));
-}
-
-void
-MaintenanceController::registerJob(Executor & executor, IMaintenanceJob::UP job)
+MaintenanceController::registerJob(IMaintenanceJob::UP job)
 {
     // Called by master write thread
     Guard guard(_jobsLock);
-    _jobs.push_back(std::make_shared<MaintenanceJobRunner>(executor, std::move(job)));
+    _jobs.push_back(std::make_shared<MaintenanceJobRunner>(_masterThread, std::move(job)));
 }
 
 void
@@ -89,11 +75,6 @@ MaintenanceController::killJobs()
     for (auto &job : _jobs) {
         job->stop(); // Make sure no more tasks are added to the executor
     }
-    for (auto &job : _jobs) {
-        while (isRunnable(*job, &_masterThread)) {
-            std::this_thread::sleep_for(1ms);
-        }
-    }
     JobList tmpJobs;
     {
         Guard guard(_jobsLock);
@@ -162,14 +143,13 @@ MaintenanceController::restart()
     if (!getStarted() || getStopping() || !_readySubDB.valid()) {
         return;
     }
-    _periodicTaskHandles.clear();
-
     addJobsToPeriodicTimer();
 }
 
 void
 MaintenanceController::addJobsToPeriodicTimer()
 {
+    _periodicTaskHandles.clear();
     // No need to take _jobsLock as modification of _jobs also happens in master write thread.
     for (const auto &jw : _jobs) {
         const IMaintenanceJob &job = jw->getJob();
diff --git a/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.h b/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.h
index af2f7e8a0fc..08e9d9c1e80 100644
--- a/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.h
+++ b/searchcore/src/vespa/searchcore/proton/server/maintenancecontroller.h
@@ -45,7 +45,7 @@ public:
                           vespalib::MonitoredRefCount& refCount, const DocTypeName& docTypeName);
 
     ~MaintenanceController();
-    void registerJobInMasterThread(IMaintenanceJob::UP job);
+    void registerJob(IMaintenanceJob::UP job);
 
     void killJobs();
 
@@ -96,7 +96,6 @@ private:
     void addJobsToPeriodicTimer();
     void restart();
     void performHoldJobs(JobList jobs);
-    void registerJob(vespalib::Executor & executor, IMaintenanceJob::UP job);
 };
 
 } // namespace proton
diff --git a/searchcore/src/vespa/searchcore/proton/server/maintenancejobrunner.h b/searchcore/src/vespa/searchcore/proton/server/maintenancejobrunner.h
index 17f244e6621..200f0d05de0 100644
--- a/searchcore/src/vespa/searchcore/proton/server/maintenancejobrunner.h
+++ b/searchcore/src/vespa/searchcore/proton/server/maintenancejobrunner.h
@@ -32,7 +32,6 @@ public:
     void run() override;
     void stop();
     bool isRunnable() const;
-    const vespalib::Executor & getExecutor() const { return _executor; }
     const IMaintenanceJob &getJob() const { return *_job; }
     IMaintenanceJob &getJob() { return *_job; }
 };