In order to ensure that lid stats are sampled at the right time ensure that you do it the *next* time you are in the

master thread. This is to ensure that the sync call has taken effect.
This also keep the iterator creation logic in one method enhancing readability.

9 files changed, 39 insertions, 26 deletions

diff --git a/searchcore/src/tests/proton/documentdb/lid_space_compaction/lid_space_compaction_test.cpp b/searchcore/src/tests/proton/documentdb/lid_space_compaction/lid_space_compaction_test.cpp
index 688dd963f61..32b2ee28f17 100644
--- a/searchcore/src/tests/proton/documentdb/lid_space_compaction/lid_space_compaction_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/lid_space_compaction/lid_space_compaction_test.cpp
@@ -279,6 +279,6 @@ TEST_P(MaxOutstandingJobTest, job_is_blocked_if_it_has_too_many_outstanding_move
 
 VESPA_GTEST_INSTANTIATE_TEST_SUITE_P(bool, JobTest, ::testing::Values(false, true));
 VESPA_GTEST_INSTANTIATE_TEST_SUITE_P(bool, JobDisabledByRemoveOpsTest, ::testing::Values(false, true));
-VESPA_GTEST_INSTANTIATE_TEST_SUITE_P(bool, MaxOutstandingJobTest, ::testing::Values(false, true));
+VESPA_GTEST_INSTANTIATE_TEST_SUITE_P(bool, MaxOutstandingJobTest, ::testing::Values(true));
 
 GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/searchcore/src/tests/proton/documentdb/move_operation_limiter/move_operation_limiter_test.cpp b/searchcore/src/tests/proton/documentdb/move_operation_limiter/move_operation_limiter_test.cpp
index 92ec0d6185b..eaad2ac2576 100644
--- a/searchcore/src/tests/proton/documentdb/move_operation_limiter/move_operation_limiter_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/move_operation_limiter/move_operation_limiter_test.cpp
@@ -54,6 +54,15 @@ struct Fixture {
     }
 };
 
+TEST_F("require that hasPending reflects if any jobs are outstanding", Fixture)
+{
+    EXPECT_FALSE(f.limiter->hasPending());
+    f.beginOp();
+    EXPECT_TRUE(f.limiter->hasPending());
+    f.endOp();
+    EXPECT_FALSE(f.limiter->hasPending());
+}
+
 TEST_F("require that job is blocked / unblocked when crossing max outstanding ops boundaries", Fixture)
 {
     f.beginOp();
diff --git a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job.cpp b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job.cpp
index e5eebca32eb..78efdaebe16 100644
--- a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job.cpp
@@ -39,7 +39,7 @@ LidSpaceCompactionJob::scanDocuments(const LidUsageStats &stats)
             }
         }
     }
-    return scanDocumentsPost();
+    return false;
 }
 
 LidSpaceCompactionJob::LidSpaceCompactionJob(const DocumentDBLidSpaceCompactionConfig &config,
diff --git a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.cpp b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.cpp
index bb6308977ca..d3dae686d2d 100644
--- a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.cpp
@@ -25,16 +25,16 @@ namespace proton {
 bool
 LidSpaceCompactionJobBase::hasTooMuchLidBloat(const LidUsageStats &stats) const
 {
-    return (stats.getLidBloat() >= _cfg.getAllowedLidBloat() &&
-            stats.getLidBloatFactor() >= _cfg.getAllowedLidBloatFactor() &&
-            stats.getLidLimit() > stats.getLowestFreeLid());
+    return ((stats.getLidBloat() >= _cfg.getAllowedLidBloat()) &&
+            (stats.getLidBloatFactor() >= _cfg.getAllowedLidBloatFactor()) &&
+            (stats.getLidLimit() > stats.getLowestFreeLid()));
 }
 
 bool
 LidSpaceCompactionJobBase::shouldRestartScanDocuments(const LidUsageStats &stats) const
 {
-    return (stats.getUsedLids() + _cfg.getAllowedLidBloat()) < stats.getHighestUsedLid() &&
-        stats.getLowestFreeLid() < stats.getHighestUsedLid();
+    return ((stats.getUsedLids() + _cfg.getAllowedLidBloat()) < stats.getHighestUsedLid()) &&
+            (stats.getLowestFreeLid() < stats.getHighestUsedLid());
 }
 
 DocumentMetaData
@@ -43,21 +43,6 @@ LidSpaceCompactionJobBase::getNextDocument(const LidUsageStats &stats, bool retr
     return _scanItr->next(std::max(stats.getLowestFreeLid(), stats.getUsedLids()), retryLastDocument);
 }
 
-bool
-LidSpaceCompactionJobBase::scanDocumentsPost()
-{
-    if (!_scanItr->valid()) {
-        sync();
-        if (shouldRestartScanDocuments(_handler->getLidStatus())) {
-            _scanItr = _handler->getIterator();
-        } else {
-            _scanItr = IDocumentScanIterator::UP();
-            _shouldCompactLidSpace = true;
-        }
-    }
-    return false; // more work to do (scan documents or compact lid space)
-}
-
 void
 LidSpaceCompactionJobBase::compactLidSpace(const LidUsageStats &stats)
 {
@@ -143,6 +128,16 @@ LidSpaceCompactionJobBase::run()
             _handler->getName().c_str());
         _is_disabled = false;
     }
+
+    if (_scanItr && !_scanItr->valid()) {
+        if (shouldRestartScanDocuments(_handler->getLidStatus())) {
+            _scanItr = _handler->getIterator();
+        } else {
+            _scanItr = IDocumentScanIterator::UP();
+            _shouldCompactLidSpace = true;
+        }
+    }
+
     if (_scanItr) {
         return scanDocuments(stats);
     } else if (_shouldCompactLidSpace) {
diff --git a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.h b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.h
index dc9ecff22cc..759a18361f7 100644
--- a/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.h
+++ b/searchcore/src/vespa/searchcore/proton/server/lid_space_compaction_job_base.h
@@ -52,8 +52,6 @@ private:
     bool remove_is_ongoing() const;
 protected:
     search::DocumentMetaData getNextDocument(const search::LidUsageStats &stats, bool retryLastDocument);
-    bool scanDocumentsPost();
-    virtual void sync() { }
 public:
     LidSpaceCompactionJobBase(const DocumentDBLidSpaceCompactionConfig &config,
                               std::shared_ptr<ILidSpaceCompactionHandler> handler,
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 faeb5da1f38..6e1b8ed79b5 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
@@ -39,7 +39,10 @@ CompactionJob::scanDocuments(const LidUsageStats &stats)
             }
         }
     }
-    return scanDocumentsPost();
+    if (!_scanItr->valid()) {
+        sync();
+    }
+    return false;
 }
 
 void
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 e5619720825..35e02246255 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
@@ -31,7 +31,7 @@ private:
     bool scanDocuments(const search::LidUsageStats &stats) override;
     void moveDocument(const search::DocumentMetaData & meta, std::shared_ptr<IDestructorCallback> onDone);
     void onStop() override;
-    void sync() override;
+    void sync();
 public:
     CompactionJob(const DocumentDBLidSpaceCompactionConfig &config,
                   std::shared_ptr<ILidSpaceCompactionHandler> handler,
diff --git a/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.cpp b/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.cpp
index 42fe492539c..e3d565afb17 100644
--- a/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.cpp
@@ -57,6 +57,13 @@ MoveOperationLimiter::isAboveLimit() const
     return (_outstandingOps >= _maxOutstandingOps);
 }
 
+bool
+MoveOperationLimiter::hasPending() const
+{
+    LockGuard guard(_mutex);
+    return (_outstandingOps > 0);
+}
+
 std::shared_ptr<vespalib::IDestructorCallback>
 MoveOperationLimiter::beginOperation()
 {
diff --git a/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.h b/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.h
index 04440a7451d..01737336a58 100644
--- a/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.h
+++ b/searchcore/src/vespa/searchcore/proton/server/move_operation_limiter.h
@@ -38,6 +38,7 @@ public:
     ~MoveOperationLimiter();
     void clearJob();
     bool isAboveLimit() const;
+    bool hasPending() const;
     std::shared_ptr<vespalib::IDestructorCallback> beginOperation() override;
 };