- Remove multibit_split form config, as it is always off, but leave it for tests.

- Reduce penetration of generated StorFilestorConfig.

16 files changed, 67 insertions, 47 deletions

diff --git a/configdefinitions/src/vespa/stor-filestor.def b/configdefinitions/src/vespa/stor-filestor.def
index e064cbefb46..de67d4336e9 100644
--- a/configdefinitions/src/vespa/stor-filestor.def
+++ b/configdefinitions/src/vespa/stor-filestor.def
@@ -29,12 +29,6 @@ response_sequencer_type enum {LATENCY, THROUGHPUT, ADAPTIVE} default=ADAPTIVE re
 ## Should follow stor-distributormanager:splitsize (16MB).
 bucket_merge_chunk_size int default=16772216 restart
 
-## Whether or not to enable the multibit split optimalization. This is useful
-## if splitting is expensive, but listing document identifiers is fairly cheap.
-## This is true for memfile persistence layer, but not for vespa search.
-## TODO verify its usage
-enable_multibit_split_optimalization bool default=true restart
-
 ## Whether or not to use async message handling when scheduling storage messages from FileStorManager.
 ##
 ## When turned on, the calling thread (e.g. FNET network thread when using Storage API RPC)
diff --git a/storage/src/tests/bucketdb/bucketmanagertest.cpp b/storage/src/tests/bucketdb/bucketmanagertest.cpp
index 6173a43e25e..92547a83d25 100644
--- a/storage/src/tests/bucketdb/bucketmanagertest.cpp
+++ b/storage/src/tests/bucketdb/bucketmanagertest.cpp
@@ -5,7 +5,6 @@
 #include <tests/common/teststorageapp.h>
 #include <vespa/config/helper/configgetter.hpp>
 #include <vespa/document/config/config-documenttypes.h>
-#include <vespa/document/config/documenttypes_config_fwd.h>
 #include <vespa/document/datatype/documenttype.h>
 #include <vespa/document/fieldvalue/document.h>
 #include <vespa/document/repo/documenttyperepo.h>
@@ -24,6 +23,7 @@
 #include <vespa/vdslib/state/random.h>
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/stllike/asciistream.h>
+#include <vespa/config-stor-filestor.h>
 #include <future>
 
 #include <vespa/log/log.h>
@@ -158,7 +158,7 @@ void BucketManagerTest::setupTestEnvironment(bool fakePersistenceLayer, bool noD
         _bottom = bottom.get();
         _top->push_back(std::move(bottom));
     } else {
-        using vespa::config::content::StorFilestorConfig;
+        using StorFilestorConfig = vespa::config::content::internal::InternalStorFilestorType;
         auto bottom = std::make_unique<FileStorManager>(*config_from<StorFilestorConfig>(config_uri),
                                                         _node->getPersistenceProvider(), _node->getComponentRegister(),
                                                         *_node, _node->get_host_info());
diff --git a/storage/src/tests/persistence/common/filestortestfixture.cpp b/storage/src/tests/persistence/common/filestortestfixture.cpp
index 86015bee4e7..6581fb9f7b1 100644
--- a/storage/src/tests/persistence/common/filestortestfixture.cpp
+++ b/storage/src/tests/persistence/common/filestortestfixture.cpp
@@ -12,6 +12,7 @@
 #include <vespa/storage/persistence/messages.h>
 #include <vespa/storageapi/message/bucket.h>
 #include <vespa/vdslib/state/clusterstate.h>
+#include <vespa/config-stor-filestor.h>
 #include <sstream>
 
 using storage::spi::test::makeSpiBucket;
@@ -75,7 +76,7 @@ FileStorTestFixture::TestFileStorComponents::TestFileStorComponents(
       manager(nullptr)
 {
     injector.inject(top);
-    using vespa::config::content::StorFilestorConfig;
+    using StorFilestorConfig = vespa::config::content::internal::InternalStorFilestorType;
     auto config = config_from<StorFilestorConfig>(config::ConfigUri(fixture._config->getConfigId()));
     auto fsm = std::make_unique<FileStorManager>(*config, fixture._node->getPersistenceProvider(),
                                                  fixture._node->getComponentRegister(), *fixture._node, fixture._node->get_host_info());
diff --git a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp
index 4846c90397a..1ccd51d3f06 100644
--- a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp
+++ b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp
@@ -34,6 +34,7 @@
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/util/gate.h>
 #include <vespa/vespalib/util/size_literals.h>
+#include <vespa/config-stor-filestor.h>
 #include <atomic>
 #include <thread>
 
@@ -46,7 +47,7 @@ using namespace storage::api;
 using storage::spi::test::makeSpiBucket;
 using document::test::makeDocumentBucket;
 using vespalib::IDestructorCallback;
-using vespa::config::content::StorFilestorConfig;
+using StorFilestorConfig = vespa::config::content::internal::InternalStorFilestorType;
 using vespa::config::content::StorFilestorConfigBuilder;
 using namespace ::testing;
 
@@ -281,7 +282,7 @@ struct PersistenceHandlerComponents : public FileStorHandlerComponents {
     {
         StorFilestorConfig cfg;
         persistenceHandler =
-                std::make_unique<PersistenceHandler>(executor, component, cfg,
+                std::make_unique<PersistenceHandler>(executor, component, 4_Mi, false,
                                                      test._node->getPersistenceProvider(),
                                                      *filestorHandler, bucketOwnershipNotifier,
                                                      *metrics.threads[0]);
@@ -746,12 +747,11 @@ TEST_F(FileStorManagerTest, priority) {
 
     ServiceLayerComponent component(_node->getComponentRegister(), "test");
     BucketOwnershipNotifier bucketOwnershipNotifier(component, c.messageSender);
-    StorFilestorConfig cfg;
-    PersistenceHandler persistenceHandler(_node->executor(), component, cfg, _node->getPersistenceProvider(),
+    PersistenceHandler persistenceHandler(_node->executor(), component, 4_Mi, false, _node->getPersistenceProvider(),
                                           filestorHandler, bucketOwnershipNotifier, *metrics.threads[0]);
     std::unique_ptr<DiskThread> thread(createThread(persistenceHandler, filestorHandler, component));
 
-    PersistenceHandler persistenceHandler2(_node->executor(), component, cfg, _node->getPersistenceProvider(),
+    PersistenceHandler persistenceHandler2(_node->executor(), component, 4_Mi, false, _node->getPersistenceProvider(),
                                            filestorHandler, bucketOwnershipNotifier, *metrics.threads[1]);
     std::unique_ptr<DiskThread> thread2(createThread(persistenceHandler2, filestorHandler, component));
 
diff --git a/storage/src/tests/persistence/persistencetestutils.cpp b/storage/src/tests/persistence/persistencetestutils.cpp
index d215448dfbc..a599b1d380a 100644
--- a/storage/src/tests/persistence/persistencetestutils.cpp
+++ b/storage/src/tests/persistence/persistencetestutils.cpp
@@ -14,7 +14,6 @@
 #include <vespa/vespalib/objects/nbostream.h>
 #include <vespa/vespalib/util/exceptions.h>
 #include <vespa/vespalib/util/sequencedtaskexecutor.h>
-#include <vespa/config-stor-filestor.h>
 #include <filesystem>
 #include <thread>
 
@@ -45,6 +44,9 @@ struct ConfigReader : public T::Subscriber
     void configure(const T& c) { config = c; }
 };
 
+constexpr bool ENABLE_MULTIBIT_SPLIT = true;
+constexpr uint32_t MERGE_CHUNK_SIZE = 4_Mi;
+
 }
 
 PersistenceTestEnvironment::PersistenceTestEnvironment(const std::string & rootOfRoot)
@@ -111,8 +113,8 @@ PersistenceTestUtils::PersistenceTestUtils()
       _persistenceHandler()
 {
     setupExecutor(1);
-    vespa::config::content::StorFilestorConfig cfg;
-    _persistenceHandler = std::make_unique<PersistenceHandler>(*_sequenceTaskExecutor, _env->_component, cfg,
+    _persistenceHandler = std::make_unique<PersistenceHandler>(*_sequenceTaskExecutor, _env->_component,
+                                                               MERGE_CHUNK_SIZE, ENABLE_MULTIBIT_SPLIT,
                                                                getPersistenceProvider(), getEnv()._fileStorHandler,
                                                                _bucketOwnershipNotifier, getEnv()._metrics);
 }
diff --git a/storage/src/tests/persistence/persistencethread_splittest.cpp b/storage/src/tests/persistence/persistencethread_splittest.cpp
index 41e86dfd160..b9f012e046c 100644
--- a/storage/src/tests/persistence/persistencethread_splittest.cpp
+++ b/storage/src/tests/persistence/persistencethread_splittest.cpp
@@ -224,9 +224,9 @@ PersistenceThreadSplitTest::doTest(SplitCase splitCase)
         expected.insert(ost.str());
     }
     std::set<std::string> actual;
-    for (uint32_t i=0; i<reply.getSplitInfo().size(); ++i) {
+    for (auto & info : reply.getSplitInfo()) {
         std::ostringstream ost;
-        document::BucketId b(reply.getSplitInfo()[i].first);
+        document::BucketId b(info.first);
         ost << b << " - " << b.getUsedBits();
         actual.insert(ost.str());
     }
diff --git a/storage/src/tests/visiting/visitormanagertest.cpp b/storage/src/tests/visiting/visitormanagertest.cpp
index 64d2042b61a..09457038b70 100644
--- a/storage/src/tests/visiting/visitormanagertest.cpp
+++ b/storage/src/tests/visiting/visitormanagertest.cpp
@@ -22,6 +22,7 @@
 #include <vespa/config/common/exceptions.h>
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/objects/nbostream.h>
+#include <vespa/config-stor-filestor.h>
 #include <gmock/gmock.h>
 #include <optional>
 #include <thread>
@@ -98,7 +99,7 @@ VisitorManagerTest::initializeTest(bool defer_manager_thread_start)
                                                defer_manager_thread_start);
     _manager = vm.get();
     _top->push_back(std::move(vm));
-    using vespa::config::content::StorFilestorConfig;
+    using StorFilestorConfig = vespa::config::content::internal::InternalStorFilestorType;
     auto filestor_cfg = config_from<StorFilestorConfig>(config::ConfigUri(config.getConfigId()));
     _top->push_back(std::make_unique<FileStorManager>(*filestor_cfg, _node->getPersistenceProvider(),
                                                       _node->getComponentRegister(), *_node, _node->get_host_info()));
diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp
index 093c11fb913..00c72a17933 100644
--- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp
+++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp
@@ -25,6 +25,7 @@
 #include <vespa/vespalib/util/sequencedtaskexecutor.h>
 #include <vespa/vespalib/util/string_escape.h>
 #include <vespa/vespalib/util/stringfmt.h>
+#include <vespa/config-stor-filestor.h>
 #include <vespa/config/helper/configfetcher.hpp>
 #include <thread>
 
@@ -180,6 +181,8 @@ computeAllPossibleHandlerThreads(const vespa::config::content::StorFilestorConfi
            1; // Async cluster state processing thread (might be a pessimization to include here...)
 }
 
+constexpr bool DISABLE_MULTIBIT_SPLIT = false;
+
 }
 
 PersistenceHandler &
@@ -189,8 +192,8 @@ FileStorManager::createRegisteredHandler(const ServiceLayerComponent & component
     size_t index = _persistenceHandlers.size();
     assert(index < _metrics->threads.size());
     _persistenceHandlers.push_back(
-            std::make_unique<PersistenceHandler>(*_sequencedExecutor, component,
-                                                 *_config, *_provider, *_filestorHandler,
+            std::make_unique<PersistenceHandler>(*_sequencedExecutor, component, _config->bucketMergeChunkSize,
+                                                 DISABLE_MULTIBIT_SPLIT, *_provider, *_filestorHandler,
                                                  *_bucketOwnershipNotifier, *_metrics->threads[index]));
     return *_persistenceHandlers.back();
 }
diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.h b/storage/src/vespa/storage/persistence/filestorage/filestormanager.h
index 96cff8dfeee..e68f8bfe27b 100644
--- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.h
+++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.h
@@ -12,7 +12,6 @@
 #include <vespa/storage/common/servicelayercomponent.h>
 #include <vespa/storage/common/statusmessages.h>
 #include <vespa/storage/common/storagelinkqueued.h>
-#include <vespa/config-stor-filestor.h>
 #include <vespa/storage/persistence/diskthread.h>
 
 #include <vespa/storage/common/nodestateupdater.h>
@@ -26,6 +25,10 @@ namespace config {
 }
 namespace vespalib { class IDestructorCallback; }
 
+namespace vespa::config::content::internal {
+    class InternalStorFilestorType;
+}
+
 namespace storage {
 namespace api {
     class ReturnCode;
@@ -50,7 +53,7 @@ class FileStorManager : public StorageLinkQueued,
                         public MessageSender,
                         public spi::BucketExecutor
 {
-    using StorFilestorConfig = vespa::config::content::StorFilestorConfig;
+    using StorFilestorConfig = vespa::config::content::internal::InternalStorFilestorType;
 
     ServiceLayerComponentRegister             & _compReg;
     ServiceLayerComponent                       _component;
@@ -62,10 +65,10 @@ class FileStorManager : public StorageLinkQueued,
     std::vector<std::unique_ptr<DiskThread>>         _threads;
     std::unique_ptr<BucketOwnershipNotifier>         _bucketOwnershipNotifier;
 
-    std::unique_ptr<vespa::config::content::StorFilestorConfig> _config;
-    bool                  _use_async_message_handling_on_schedule;
-    std::shared_ptr<FileStorMetrics> _metrics;
-    std::unique_ptr<FileStorHandler> _filestorHandler;
+    std::unique_ptr<StorFilestorConfig> _config;
+    bool                                _use_async_message_handling_on_schedule;
+    std::shared_ptr<FileStorMetrics>    _metrics;
+    std::unique_ptr<FileStorHandler>    _filestorHandler;
     std::unique_ptr<vespalib::ISequencedTaskExecutor> _sequencedExecutor;
 
     bool       _closed;
@@ -111,7 +114,7 @@ public:
 
     const FileStorMetrics& get_metrics() const { return *_metrics; }
 
-    void on_configure(const vespa::config::content::StorFilestorConfig& config);
+    void on_configure(const StorFilestorConfig& config);
 
 private:
     PersistenceHandler & createRegisteredHandler(const ServiceLayerComponent & component);
diff --git a/storage/src/vespa/storage/persistence/persistencehandler.cpp b/storage/src/vespa/storage/persistence/persistencehandler.cpp
index 87c1f83794e..0612798b43a 100644
--- a/storage/src/vespa/storage/persistence/persistencehandler.cpp
+++ b/storage/src/vespa/storage/persistence/persistencehandler.cpp
@@ -11,7 +11,8 @@ namespace storage {
 
 PersistenceHandler::PersistenceHandler(vespalib::ISequencedTaskExecutor & sequencedExecutor,
                                       const ServiceLayerComponent & component,
-                                      const vespa::config::content::StorFilestorConfig & cfg,
+                                      uint32_t bucketMergeChunkSize,
+                                      bool multibitSplit,
                                       spi::PersistenceProvider& provider,
                                       FileStorHandler& filestorHandler,
                                       BucketOwnershipNotifier & bucketOwnershipNotifier,
@@ -19,9 +20,9 @@ PersistenceHandler::PersistenceHandler(vespalib::ISequencedTaskExecutor & sequen
     : _clock(component.getClock()),
       _env(component, filestorHandler, metrics, provider),
       _processAllHandler(_env, provider),
-      _mergeHandler(_env, provider, component.cluster_context(), _clock, sequencedExecutor, cfg.bucketMergeChunkSize),
+      _mergeHandler(_env, provider, component.cluster_context(), _clock, sequencedExecutor, bucketMergeChunkSize),
       _asyncHandler(_env, provider, bucketOwnershipNotifier, sequencedExecutor, component.getBucketIdFactory()),
-      _splitJoinHandler(_env, provider, bucketOwnershipNotifier, cfg.enableMultibitSplitOptimalization),
+      _splitJoinHandler(_env, provider, bucketOwnershipNotifier, multibitSplit),
       _simpleHandler(_env, provider, component.getBucketIdFactory())
 {
 }
diff --git a/storage/src/vespa/storage/persistence/persistencehandler.h b/storage/src/vespa/storage/persistence/persistencehandler.h
index 1835b56528e..0da518a1cfa 100644
--- a/storage/src/vespa/storage/persistence/persistencehandler.h
+++ b/storage/src/vespa/storage/persistence/persistencehandler.h
@@ -10,8 +10,6 @@
 #include "simplemessagehandler.h"
 #include <vespa/storage/common/storagecomponent.h>
 #include <vespa/vespalib/util/isequencedtaskexecutor.h>
-#include <vespa/config-stor-filestor.h>
-#include <atomic>
 
 namespace storage {
 
@@ -26,7 +24,7 @@ class PersistenceHandler : public Types
 {
 public:
     PersistenceHandler(vespalib::ISequencedTaskExecutor &, const ServiceLayerComponent & component,
-                      const vespa::config::content::StorFilestorConfig &, spi::PersistenceProvider &,
+                      uint32_t mergeChunkSize, bool multibitSplit, spi::PersistenceProvider &,
                       FileStorHandler &, BucketOwnershipNotifier &, FileStorThreadMetrics&);
     ~PersistenceHandler();
 
diff --git a/storage/src/vespa/storage/persistence/splitjoinhandler.h b/storage/src/vespa/storage/persistence/splitjoinhandler.h
index af7602b8ec5..dd096fef863 100644
--- a/storage/src/vespa/storage/persistence/splitjoinhandler.h
+++ b/storage/src/vespa/storage/persistence/splitjoinhandler.h
@@ -33,6 +33,7 @@ private:
     PersistenceUtil          &_env;
     spi::PersistenceProvider &_spi;
     BucketOwnershipNotifier  &_bucketOwnershipNotifier;
+    // TODO Rewrite tests so below config can be removed. Hardcoded to false in production.
     bool                      _enableMultibitSplitOptimalization;
 };
 
diff --git a/storage/src/vespa/storage/storageserver/servicelayernode.cpp b/storage/src/vespa/storage/storageserver/servicelayernode.cpp
index 7da75225b6c..d18935afe24 100644
--- a/storage/src/vespa/storage/storageserver/servicelayernode.cpp
+++ b/storage/src/vespa/storage/storageserver/servicelayernode.cpp
@@ -18,6 +18,9 @@
 #include <vespa/vespalib/util/exceptions.h>
 #include <vespa/messagebus/rpcmessagebus.h>
 #include <vespa/config/common/exceptions.h>
+#include <vespa/config-stor-filestor.h>
+#include <vespa/config-persistence.h>
+#include <vespa/storage/visiting/config-stor-visitor.h>
 
 #include <vespa/log/log.h>
 LOG_SETUP(".node.servicelayer");
diff --git a/storage/src/vespa/storage/storageserver/servicelayernode.h b/storage/src/vespa/storage/storageserver/servicelayernode.h
index bea09a1c9ce..b0fa8a6f9ac 100644
--- a/storage/src/vespa/storage/storageserver/servicelayernode.h
+++ b/storage/src/vespa/storage/storageserver/servicelayernode.h
@@ -6,14 +6,19 @@
 #include "servicelayernodecontext.h"
 #include "storagenode.h"
 #include "vespa/vespalib/util/jsonstream.h"
-#include <vespa/config-persistence.h>
-#include <vespa/config-stor-filestor.h>
 #include <vespa/storage/common/nodestateupdater.h>
 #include <vespa/storage/common/visitorfactory.h>
-#include <vespa/storage/visiting/config-stor-visitor.h>
 #include <vespa/storage/visiting/visitormessagesessionfactory.h>
 #include <vespa/vespalib/util/hw_info.h>
 
+namespace vespa::config::content::internal {
+    class InternalStorFilestorType;
+    class InternalPersistenceType;
+}
+namespace vespa::config::content::core::internal {
+    class InternalStorVisitorType;
+}
+
 namespace storage {
 
 namespace spi { struct PersistenceProvider; }
@@ -33,9 +38,9 @@ class ServiceLayerNode
 
 {
 public:
-    using PersistenceConfig  = vespa::config::content::PersistenceConfig;
-    using StorVisitorConfig  = vespa::config::content::core::StorVisitorConfig;
-    using StorFilestorConfig = vespa::config::content::StorFilestorConfig;
+    using PersistenceConfig  = vespa::config::content::internal::InternalPersistenceType;
+    using StorVisitorConfig  = vespa::config::content::core::internal::InternalStorVisitorType;
+    using StorFilestorConfig = vespa::config::content::internal::InternalStorFilestorType;
 private:
     ServiceLayerNodeContext&            _context;
     spi::PersistenceProvider&           _persistenceProvider;
diff --git a/storageserver/src/vespa/storageserver/app/servicelayerprocess.cpp b/storageserver/src/vespa/storageserver/app/servicelayerprocess.cpp
index ebf320352eb..abeaf06b4ad 100644
--- a/storageserver/src/vespa/storageserver/app/servicelayerprocess.cpp
+++ b/storageserver/src/vespa/storageserver/app/servicelayerprocess.cpp
@@ -5,6 +5,9 @@
 #include <vespa/storage/common/content_bucket_db_options.h>
 #include <vespa/storage/common/i_storage_chain_builder.h>
 #include <vespa/storage/config/config-stor-server.h>
+#include <vespa/config-persistence.h>
+#include <vespa/config-stor-filestor.h>
+#include <vespa/storage/visiting/config-stor-visitor.h>
 #include <vespa/storage/storageserver/servicelayernode.h>
 #include <vespa/storageframework/defaultimplementation/clock/realclock.h>
 #include <vespa/searchvisitor/searchvisitor.h>
diff --git a/storageserver/src/vespa/storageserver/app/servicelayerprocess.h b/storageserver/src/vespa/storageserver/app/servicelayerprocess.h
index add5a38ca9d..553fff2d55f 100644
--- a/storageserver/src/vespa/storageserver/app/servicelayerprocess.h
+++ b/storageserver/src/vespa/storageserver/app/servicelayerprocess.h
@@ -2,15 +2,20 @@
 #pragma once
 
 #include "process.h"
-#include <vespa/config-persistence.h>
-#include <vespa/config-stor-filestor.h>
 #include <vespa/storage/common/visitorfactory.h>
 #include <vespa/storage/storageserver/servicelayernodecontext.h>
-#include <vespa/storage/visiting/config-stor-visitor.h>
 #include <vespa/vespalib/util/hw_info.h>
 
 namespace config { class ConfigUri; }
 
+namespace vespa::config::content::internal {
+    class InternalStorFilestorType;
+    class InternalPersistenceType;
+}
+namespace vespa::config::content::core::internal {
+    class InternalStorVisitorType;
+}
+
 namespace storage {
 
 namespace spi { struct PersistenceProvider; }
@@ -22,9 +27,9 @@ class ServiceLayerProcess : public Process {
 protected:
     VisitorFactory::Map _externalVisitors;
 private:
-    using PersistenceConfig  = vespa::config::content::PersistenceConfig;
-    using StorVisitorConfig  = vespa::config::content::core::StorVisitorConfig;
-    using StorFilestorConfig = vespa::config::content::StorFilestorConfig;
+    using PersistenceConfig  = vespa::config::content::internal::InternalPersistenceType;
+    using StorVisitorConfig  = vespa::config::content::core::internal::InternalStorVisitorType;
+    using StorFilestorConfig = vespa::config::content::internal::InternalStorFilestorType;
 
     std::unique_ptr<config::ConfigHandle<PersistenceConfig>>  _persistence_cfg_handle;
     std::unique_ptr<config::ConfigHandle<StorVisitorConfig>>  _visitor_cfg_handle;