Hardcode visitor iterators per bucket to 1

We have an SPI iterate() invariant that states the provider shall never
observe concurrent calls for the same iterator ID. If GetIter operations
can operate in shared locking mode, this invariant will no longer hold
if multiple GetIters for a single iterator ID can be pipelined in the
queue. We therefore ignore the config entirely.

This is not expected to cause any performance regressions in practice.

3 files changed, 19 insertions, 30 deletions

diff --git a/storage/src/tests/visiting/visitortest.cpp b/storage/src/tests/visiting/visitortest.cpp
index 27281d9b95f..4fc577226ca 100644
--- a/storage/src/tests/visiting/visitortest.cpp
+++ b/storage/src/tests/visiting/visitortest.cpp
@@ -62,7 +62,7 @@ private:
     CPPUNIT_TEST(testNormalUsage);
     CPPUNIT_TEST(testFailedCreateIterator);
     CPPUNIT_TEST(testFailedGetIter);
-    CPPUNIT_TEST(testMultipleFailedGetIter);
+    CPPUNIT_TEST(iterators_per_bucket_config_is_ignored_and_hardcoded_to_1);
     CPPUNIT_TEST(testDocumentAPIClientError);
     CPPUNIT_TEST(testNoDocumentAPIResendingForFailedVisitor);
     CPPUNIT_TEST(testIteratorCreatedForFailedVisitor);
@@ -90,7 +90,7 @@ public:
     void testNormalUsage();
     void testFailedCreateIterator();
     void testFailedGetIter();
-    void testMultipleFailedGetIter();
+    void iterators_per_bucket_config_is_ignored_and_hardcoded_to_1();
     void testDocumentAPIClientError();
     void testNoDocumentAPIResendingForFailedVisitor();
     void testIteratorCreatedForFailedVisitor();
@@ -592,36 +592,31 @@ VisitorTest::testFailedGetIter()
     CPPUNIT_ASSERT(waitUntilNoActiveVisitors());
 }
 
-void
-VisitorTest::testMultipleFailedGetIter()
-{
-    initializeTest(TestParams().iteratorsPerBucket(2));
-    std::shared_ptr<api::CreateVisitorCommand> cmd(
-            makeCreateVisitor());
+void VisitorTest::iterators_per_bucket_config_is_ignored_and_hardcoded_to_1() {
+    initializeTest(TestParams().iteratorsPerBucket(20));
+    auto cmd = makeCreateVisitor();
     _top->sendDown(cmd);
     sendCreateIteratorReply();
 
-    std::vector<GetIterCommand::SP> getIterCmds(
-            fetchMultipleCommands<GetIterCommand>(*_bottom, 2));
-
-    sendGetIterReply(*getIterCmds[0],
-                     api::ReturnCode(api::ReturnCode::BUCKET_NOT_FOUND));
-
-    // Wait for an "appropriate" amount of time so that wrongful logic
-    // will send a DestroyIteratorCommand before all pending GetIters
-    // have been replied to.
-    std::this_thread::sleep_for(100ms);
+    auto getIterCmd = fetchSingleCommand<GetIterCommand>(*_bottom);
+    CPPUNIT_ASSERT_EQUAL(spi::IteratorId(1234),
+                         getIterCmd->getIteratorId());
+    sendGetIterReply(*getIterCmd);
 
     CPPUNIT_ASSERT_EQUAL(size_t(0), _bottom->getNumCommands());
 
-    sendGetIterReply(*getIterCmds[1],
-                     api::ReturnCode(api::ReturnCode::BUCKET_DELETED));
+    std::vector<document::Document::SP> docs;
+    std::vector<document::DocumentId> docIds;
+    std::vector<std::string> infoMessages;
+    getMessagesAndReply(_documents.size(), getSession(0), docs, docIds, infoMessages);
+    CPPUNIT_ASSERT_EQUAL(size_t(0), infoMessages.size());
+    CPPUNIT_ASSERT_EQUAL(size_t(0), docIds.size());
 
-    DestroyIteratorCommand::SP destroyIterCmd(
-            fetchSingleCommand<DestroyIteratorCommand>(*_bottom));
+    auto destroyIterCmd = fetchSingleCommand<DestroyIteratorCommand>(*_bottom);
 
-    verifyCreateVisitorReply(api::ReturnCode::BUCKET_DELETED, 0, 0);
+    verifyCreateVisitorReply(api::ReturnCode::OK);
     CPPUNIT_ASSERT(waitUntilNoActiveVisitors());
+    CPPUNIT_ASSERT_EQUAL(0L, getFailedVisitorDestinationReplyCount());
 }
 
 void
diff --git a/storage/src/vespa/storage/visiting/stor-visitor.def b/storage/src/vespa/storage/visiting/stor-visitor.def
index 1e80f2993a5..6f16bcb60a2 100644
--- a/storage/src/vespa/storage/visiting/stor-visitor.def
+++ b/storage/src/vespa/storage/visiting/stor-visitor.def
@@ -24,6 +24,7 @@ defaultparalleliterators int default=8
 ## will be 16 requests to persistence layer, but only 8 will be able to execute
 ## at the same time, since only one operation can be executed at the same time
 ## for one bucket)
+## DEPRECATED: ignored by backend, 1 is always used.
 iterators_per_bucket int default=1
 
 ## Default number of maximum client replies pending.
diff --git a/storage/src/vespa/storage/visiting/visitorthread.cpp b/storage/src/vespa/storage/visiting/visitorthread.cpp
index a8f31514eb1..b12a1eb6e7f 100644
--- a/storage/src/vespa/storage/visiting/visitorthread.cpp
+++ b/storage/src/vespa/storage/visiting/visitorthread.cpp
@@ -637,7 +637,6 @@ VisitorThread::onInternal(const std::shared_ptr<api::InternalCommand>& cmd)
             _ignoreNonExistingVisitorTimeLimit
                     = config.ignorenonexistingvisitortimelimit;
             _defaultParallelIterators = config.defaultparalleliterators;
-            _iteratorsPerBucket = config.iteratorsPerBucket;
             _defaultPendingMessages = config.defaultpendingmessages;
             _defaultDocBlockSize = config.defaultdocblocksize;
             _visitorMemoryUsageLimit = config.visitorMemoryUsageLimit;
@@ -647,12 +646,6 @@ VisitorThread::onInternal(const std::shared_ptr<api::InternalCommand>& cmd)
                 LOG(config, "Cannot use value of defaultParallelIterators < 1");
                 _defaultParallelIterators = 1;
             }
-            if (_iteratorsPerBucket < 1 && _iteratorsPerBucket > 10) {
-                if (_iteratorsPerBucket < 1) _iteratorsPerBucket = 1;
-                else _iteratorsPerBucket = 10;
-                LOG(config, "Invalid value of iterators per bucket %u using %u",
-                    config.iteratorsPerBucket, _iteratorsPerBucket);
-            }
             if (_defaultPendingMessages < 1) {
                 LOG(config, "Cannot use value of defaultPendingMessages < 1");
                 _defaultPendingMessages = 1;