aboutsummaryrefslogtreecommitdiffstats
path: root/storage
diff options
context:
space:
mode:
authorTor Brede Vekterli <vekterli@yahooinc.com>2023-10-17 09:28:20 +0000
committerTor Brede Vekterli <vekterli@yahooinc.com>2023-10-17 09:28:20 +0000
commit35795f558e8386a817f72d996a94ec2fb37a5032 (patch)
tree75a93d7c0c44ce95e241c31b89cfbb7ac0286f93 /storage
parentc11a6d32d06a115cce22c87066698f7436588030 (diff)
Remove unused configurability of operation priorities
As far as I know, this config has not been used by anyone for at least a decade (if it ever was used for anything truly useful). Additionally, operation priorities are a foot-gun at the best of times. The ability to dynamically change the meaning of priority enums even more so. This commit entirely removes configuration of Document API priority mappings in favor of a fixed mapping that is equal to the default config, i.e. what everyone's been using anyway. This removes a thread per distributor/storage node process as well as 1 mutex and 1 (presumably entirely unneeded `seq_cst`) atomic load in the message hot path. Also precomputes a LUT for the priority reverse mapping to avoid needing to lower-bound seek an explicit map.
Diffstat (limited to 'storage')
-rw-r--r--storage/src/tests/storageserver/documentapiconvertertest.cpp2
-rw-r--r--storage/src/tests/storageserver/priorityconvertertest.cpp4
-rw-r--r--storage/src/tests/storageserver/testvisitormessagesession.h6
-rw-r--r--storage/src/tests/visiting/visitormanagertest.cpp2
-rw-r--r--storage/src/tests/visiting/visitortest.cpp2
-rw-r--r--storage/src/vespa/storage/storageserver/communicationmanager.cpp2
-rw-r--r--storage/src/vespa/storage/storageserver/documentapiconverter.cpp5
-rw-r--r--storage/src/vespa/storage/storageserver/documentapiconverter.h11
-rw-r--r--storage/src/vespa/storage/storageserver/priorityconverter.cpp132
-rw-r--r--storage/src/vespa/storage/storageserver/priorityconverter.h44
10 files changed, 99 insertions, 111 deletions
diff --git a/storage/src/tests/storageserver/documentapiconvertertest.cpp b/storage/src/tests/storageserver/documentapiconvertertest.cpp
index 5e70c00cc5f..eb4789b25d4 100644
--- a/storage/src/tests/storageserver/documentapiconvertertest.cpp
+++ b/storage/src/tests/storageserver/documentapiconvertertest.cpp
@@ -77,7 +77,7 @@ struct DocumentApiConverterTest : Test {
}
void SetUp() override {
- _converter = std::make_unique<DocumentApiConverter>(config::ConfigUri("raw:"), _bucketResolver);
+ _converter = std::make_unique<DocumentApiConverter>(_bucketResolver);
};
template <typename DerivedT, typename BaseT>
diff --git a/storage/src/tests/storageserver/priorityconvertertest.cpp b/storage/src/tests/storageserver/priorityconvertertest.cpp
index 5462c83d2a2..69f9d313242 100644
--- a/storage/src/tests/storageserver/priorityconvertertest.cpp
+++ b/storage/src/tests/storageserver/priorityconvertertest.cpp
@@ -1,7 +1,6 @@
// Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
#include <vespa/storage/storageserver/priorityconverter.h>
-#include <tests/common/testhelper.h>
#include <vespa/vespalib/gtest/gtest.h>
using namespace ::testing;
@@ -12,8 +11,7 @@ struct PriorityConverterTest : Test {
std::unique_ptr<PriorityConverter> _converter;
void SetUp() override {
- vdstestlib::DirConfig config(getStandardConfig(true));
- _converter = std::make_unique<PriorityConverter>(config::ConfigUri(config.getConfigId()));
+ _converter = std::make_unique<PriorityConverter>();
};
};
diff --git a/storage/src/tests/storageserver/testvisitormessagesession.h b/storage/src/tests/storageserver/testvisitormessagesession.h
index 86d4dc92a58..cc7dab7ef9e 100644
--- a/storage/src/tests/storageserver/testvisitormessagesession.h
+++ b/storage/src/tests/storageserver/testvisitormessagesession.h
@@ -49,9 +49,11 @@ struct TestVisitorMessageSessionFactory : public VisitorMessageSessionFactory
bool _createAutoReplyVisitorSessions;
PriorityConverter _priConverter;
- TestVisitorMessageSessionFactory(vespalib::stringref configId = "")
+ TestVisitorMessageSessionFactory()
: _createAutoReplyVisitorSessions(false),
- _priConverter(config::ConfigUri(configId)) {}
+ _priConverter()
+ {
+ }
VisitorMessageSession::UP createSession(Visitor& v, VisitorThread& vt) override {
std::lock_guard lock(_accessLock);
diff --git a/storage/src/tests/visiting/visitormanagertest.cpp b/storage/src/tests/visiting/visitormanagertest.cpp
index 991b98e5489..2b7039f36ea 100644
--- a/storage/src/tests/visiting/visitormanagertest.cpp
+++ b/storage/src/tests/visiting/visitormanagertest.cpp
@@ -83,7 +83,7 @@ VisitorManagerTest::initializeTest(bool defer_manager_thread_start)
vdstestlib::DirConfig config(getStandardConfig(true));
config.getConfig("stor-visitor").set("visitorthreads", "1");
- _messageSessionFactory = std::make_unique<TestVisitorMessageSessionFactory>(config.getConfigId());
+ _messageSessionFactory = std::make_unique<TestVisitorMessageSessionFactory>();
_node = std::make_unique<TestServiceLayerApp>(config.getConfigId());
_node->setupDummyPersistence();
_node->getStateUpdater().setClusterState(std::make_shared<lib::ClusterState>("storage:1 distributor:1"));
diff --git a/storage/src/tests/visiting/visitortest.cpp b/storage/src/tests/visiting/visitortest.cpp
index 1f1d27ab4cb..49f1bc778fc 100644
--- a/storage/src/tests/visiting/visitortest.cpp
+++ b/storage/src/tests/visiting/visitortest.cpp
@@ -161,7 +161,7 @@ VisitorTest::initializeTest(const TestParams& params)
std::filesystem::create_directories(std::filesystem::path(vespalib::make_string("%s/disks/d0", rootFolder.c_str())));
std::filesystem::create_directories(std::filesystem::path(vespalib::make_string("%s/disks/d1", rootFolder.c_str())));
- _messageSessionFactory = std::make_unique<TestVisitorMessageSessionFactory>(config.getConfigId());
+ _messageSessionFactory = std::make_unique<TestVisitorMessageSessionFactory>();
if (params._autoReplyError.getCode() != mbus::ErrorCode::NONE) {
_messageSessionFactory->_autoReplyError = params._autoReplyError;
_messageSessionFactory->_createAutoReplyVisitorSessions = true;
diff --git a/storage/src/vespa/storage/storageserver/communicationmanager.cpp b/storage/src/vespa/storage/storageserver/communicationmanager.cpp
index c126ec01dc6..bbd4e87cb40 100644
--- a/storage/src/vespa/storage/storageserver/communicationmanager.cpp
+++ b/storage/src/vespa/storage/storageserver/communicationmanager.cpp
@@ -230,7 +230,7 @@ CommunicationManager::CommunicationManager(StorageComponentRegister& compReg,
_mbus(),
_configUri(configUri),
_closed(false),
- _docApiConverter(configUri, std::make_shared<PlaceHolderBucketResolver>()), // TODO wire config from outside
+ _docApiConverter(std::make_shared<PlaceHolderBucketResolver>()),
_thread()
{
_component.registerMetricUpdateHook(*this, 5s);
diff --git a/storage/src/vespa/storage/storageserver/documentapiconverter.cpp b/storage/src/vespa/storage/storageserver/documentapiconverter.cpp
index 04b3d8b6ce7..ca46e87285b 100644
--- a/storage/src/vespa/storage/storageserver/documentapiconverter.cpp
+++ b/storage/src/vespa/storage/storageserver/documentapiconverter.cpp
@@ -23,9 +23,8 @@ using document::BucketSpace;
namespace storage {
-DocumentApiConverter::DocumentApiConverter(const config::ConfigUri &configUri,
- std::shared_ptr<const BucketResolver> bucketResolver)
- : _priConverter(std::make_unique<PriorityConverter>(configUri)),
+DocumentApiConverter::DocumentApiConverter(std::shared_ptr<const BucketResolver> bucketResolver)
+ : _priConverter(std::make_unique<PriorityConverter>()),
_bucketResolver(std::move(bucketResolver))
{}
diff --git a/storage/src/vespa/storage/storageserver/documentapiconverter.h b/storage/src/vespa/storage/storageserver/documentapiconverter.h
index 5990d6f9017..96b119ff44e 100644
--- a/storage/src/vespa/storage/storageserver/documentapiconverter.h
+++ b/storage/src/vespa/storage/storageserver/documentapiconverter.h
@@ -22,18 +22,17 @@ class PriorityConverter;
class DocumentApiConverter
{
public:
- DocumentApiConverter(const config::ConfigUri &configUri,
- std::shared_ptr<const BucketResolver> bucketResolver);
+ explicit DocumentApiConverter(std::shared_ptr<const BucketResolver> bucketResolver);
~DocumentApiConverter();
- std::unique_ptr<api::StorageCommand> toStorageAPI(documentapi::DocumentMessage& msg);
- std::unique_ptr<api::StorageReply> toStorageAPI(documentapi::DocumentReply& reply, api::StorageCommand& originalCommand);
+ [[nodiscard]] std::unique_ptr<api::StorageCommand> toStorageAPI(documentapi::DocumentMessage& msg);
+ [[nodiscard]] std::unique_ptr<api::StorageReply> toStorageAPI(documentapi::DocumentReply& reply, api::StorageCommand& originalCommand);
void transferReplyState(storage::api::StorageReply& from, mbus::Reply& to);
- std::unique_ptr<mbus::Message> toDocumentAPI(api::StorageCommand& cmd);
+ [[nodiscard]] std::unique_ptr<mbus::Message> toDocumentAPI(api::StorageCommand& cmd);
const PriorityConverter& getPriorityConverter() const { return *_priConverter; }
// BucketResolver getter and setter are both thread safe.
- std::shared_ptr<const BucketResolver> bucketResolver() const;
+ [[nodiscard]] std::shared_ptr<const BucketResolver> bucketResolver() const;
void setBucketResolver(std::shared_ptr<const BucketResolver> resolver);
private:
mutable std::mutex _mutex;
diff --git a/storage/src/vespa/storage/storageserver/priorityconverter.cpp b/storage/src/vespa/storage/storageserver/priorityconverter.cpp
index 13ab572c561..fe7570ff53a 100644
--- a/storage/src/vespa/storage/storageserver/priorityconverter.cpp
+++ b/storage/src/vespa/storage/storageserver/priorityconverter.cpp
@@ -1,85 +1,91 @@
// Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
#include "priorityconverter.h"
-#include <vespa/config/subscription/configuri.h>
-#include <vespa/config/helper/configfetcher.hpp>
-
+#include <map>
namespace storage {
-PriorityConverter::PriorityConverter(const config::ConfigUri & configUri)
- : _configFetcher(std::make_unique<config::ConfigFetcher>(configUri.getContext()))
+PriorityConverter::PriorityConverter()
+ : _mapping(),
+ _reverse_mapping()
{
- _configFetcher->subscribe<vespa::config::content::core::StorPrioritymappingConfig>(configUri.getConfigId(), this);
- _configFetcher->start();
+ init_static_priority_mappings();
}
PriorityConverter::~PriorityConverter() = default;
-uint8_t
-PriorityConverter::toStoragePriority(documentapi::Priority::Value documentApiPriority) const
+void
+PriorityConverter::init_static_priority_mappings()
{
- const uint32_t index(static_cast<uint32_t>(documentApiPriority));
- if (index >= PRI_ENUM_SIZE) {
- return 255;
- }
+ // Defaults from `stor-prioritymapping` config
+ constexpr uint8_t highest = 50;
+ constexpr uint8_t very_high = 60;
+ constexpr uint8_t high_1 = 70;
+ constexpr uint8_t high_2 = 80;
+ constexpr uint8_t high_3 = 90;
+ constexpr uint8_t normal_1 = 100;
+ constexpr uint8_t normal_2 = 110;
+ constexpr uint8_t normal_3 = 120;
+ constexpr uint8_t normal_4 = 130;
+ constexpr uint8_t normal_5 = 140;
+ constexpr uint8_t normal_6 = 150;
+ constexpr uint8_t low_1 = 160;
+ constexpr uint8_t low_2 = 170;
+ constexpr uint8_t low_3 = 180;
+ constexpr uint8_t very_low = 190;
+ constexpr uint8_t lowest = 200;
- return _mapping[index];
-}
+ _mapping[documentapi::Priority::PRI_HIGHEST] = highest;
+ _mapping[documentapi::Priority::PRI_VERY_HIGH] = very_high;
+ _mapping[documentapi::Priority::PRI_HIGH_1] = high_1;
+ _mapping[documentapi::Priority::PRI_HIGH_2] = high_2;
+ _mapping[documentapi::Priority::PRI_HIGH_3] = high_3;
+ _mapping[documentapi::Priority::PRI_NORMAL_1] = normal_1;
+ _mapping[documentapi::Priority::PRI_NORMAL_2] = normal_2;
+ _mapping[documentapi::Priority::PRI_NORMAL_3] = normal_3;
+ _mapping[documentapi::Priority::PRI_NORMAL_4] = normal_4;
+ _mapping[documentapi::Priority::PRI_NORMAL_5] = normal_5;
+ _mapping[documentapi::Priority::PRI_NORMAL_6] = normal_6;
+ _mapping[documentapi::Priority::PRI_LOW_1] = low_1;
+ _mapping[documentapi::Priority::PRI_LOW_2] = low_2;
+ _mapping[documentapi::Priority::PRI_LOW_3] = low_3;
+ _mapping[documentapi::Priority::PRI_VERY_LOW] = very_low;
+ _mapping[documentapi::Priority::PRI_LOWEST] = lowest;
-documentapi::Priority::Value
-PriorityConverter::toDocumentPriority(uint8_t storagePriority) const
-{
- std::lock_guard guard(_mutex);
- std::map<uint8_t, documentapi::Priority::Value>::const_iterator iter =
- _reverseMapping.lower_bound(storagePriority);
+ std::map<uint8_t, documentapi::Priority::Value> reverse_map_helper;
+ reverse_map_helper[highest] = documentapi::Priority::PRI_HIGHEST;
+ reverse_map_helper[very_high] = documentapi::Priority::PRI_VERY_HIGH;
+ reverse_map_helper[high_1] = documentapi::Priority::PRI_HIGH_1;
+ reverse_map_helper[high_2] = documentapi::Priority::PRI_HIGH_2;
+ reverse_map_helper[high_3] = documentapi::Priority::PRI_HIGH_3;
+ reverse_map_helper[normal_1] = documentapi::Priority::PRI_NORMAL_1;
+ reverse_map_helper[normal_2] = documentapi::Priority::PRI_NORMAL_2;
+ reverse_map_helper[normal_3] = documentapi::Priority::PRI_NORMAL_3;
+ reverse_map_helper[normal_4] = documentapi::Priority::PRI_NORMAL_4;
+ reverse_map_helper[normal_5] = documentapi::Priority::PRI_NORMAL_5;
+ reverse_map_helper[normal_6] = documentapi::Priority::PRI_NORMAL_6;
+ reverse_map_helper[low_1] = documentapi::Priority::PRI_LOW_1;
+ reverse_map_helper[low_2] = documentapi::Priority::PRI_LOW_2;
+ reverse_map_helper[low_3] = documentapi::Priority::PRI_LOW_3;
+ reverse_map_helper[very_low] = documentapi::Priority::PRI_VERY_LOW;
+ reverse_map_helper[lowest] = documentapi::Priority::PRI_LOWEST;
- if (iter != _reverseMapping.end()) {
- return iter->second;
+ // Precompute a 1-1 LUT to avoid having to lower-bound lookup values in a fixed map
+ _reverse_mapping.resize(256);
+ for (size_t i = 0; i < 256; ++i) {
+ auto iter = reverse_map_helper.lower_bound(static_cast<uint8_t>(i));
+ _reverse_mapping[i] = (iter != reverse_map_helper.cend()) ? iter->second : documentapi::Priority::PRI_LOWEST;
}
-
- return documentapi::Priority::PRI_LOWEST;
}
-void
-PriorityConverter::configure(std::unique_ptr<vespa::config::content::core::StorPrioritymappingConfig> config)
+uint8_t
+PriorityConverter::toStoragePriority(documentapi::Priority::Value documentApiPriority) const noexcept
{
- // Data race free; _mapping is an array of std::atomic.
- _mapping[documentapi::Priority::PRI_HIGHEST] = config->highest;
- _mapping[documentapi::Priority::PRI_VERY_HIGH] = config->veryHigh;
- _mapping[documentapi::Priority::PRI_HIGH_1] = config->high1;
- _mapping[documentapi::Priority::PRI_HIGH_2] = config->high2;
- _mapping[documentapi::Priority::PRI_HIGH_3] = config->high3;
- _mapping[documentapi::Priority::PRI_NORMAL_1] = config->normal1;
- _mapping[documentapi::Priority::PRI_NORMAL_2] = config->normal2;
- _mapping[documentapi::Priority::PRI_NORMAL_3] = config->normal3;
- _mapping[documentapi::Priority::PRI_NORMAL_4] = config->normal4;
- _mapping[documentapi::Priority::PRI_NORMAL_5] = config->normal5;
- _mapping[documentapi::Priority::PRI_NORMAL_6] = config->normal6;
- _mapping[documentapi::Priority::PRI_LOW_1] = config->low1;
- _mapping[documentapi::Priority::PRI_LOW_2] = config->low2;
- _mapping[documentapi::Priority::PRI_LOW_3] = config->low3;
- _mapping[documentapi::Priority::PRI_VERY_LOW] = config->veryLow;
- _mapping[documentapi::Priority::PRI_LOWEST] = config->lowest;
-
- std::lock_guard guard(_mutex);
- _reverseMapping.clear();
- _reverseMapping[config->highest] = documentapi::Priority::PRI_HIGHEST;
- _reverseMapping[config->veryHigh] = documentapi::Priority::PRI_VERY_HIGH;
- _reverseMapping[config->high1] = documentapi::Priority::PRI_HIGH_1;
- _reverseMapping[config->high2] = documentapi::Priority::PRI_HIGH_2;
- _reverseMapping[config->high3] = documentapi::Priority::PRI_HIGH_3;
- _reverseMapping[config->normal1] = documentapi::Priority::PRI_NORMAL_1;
- _reverseMapping[config->normal2] = documentapi::Priority::PRI_NORMAL_2;
- _reverseMapping[config->normal3] = documentapi::Priority::PRI_NORMAL_3;
- _reverseMapping[config->normal4] = documentapi::Priority::PRI_NORMAL_4;
- _reverseMapping[config->normal5] = documentapi::Priority::PRI_NORMAL_5;
- _reverseMapping[config->normal6] = documentapi::Priority::PRI_NORMAL_6;
- _reverseMapping[config->low1] = documentapi::Priority::PRI_LOW_1;
- _reverseMapping[config->low2] = documentapi::Priority::PRI_LOW_2;
- _reverseMapping[config->low3] = documentapi::Priority::PRI_LOW_3;
- _reverseMapping[config->veryLow] = documentapi::Priority::PRI_VERY_LOW;
- _reverseMapping[config->lowest] = documentapi::Priority::PRI_LOWEST;
+ const auto index = static_cast<uint32_t>(documentApiPriority);
+ if (index >= PRI_ENUM_SIZE) {
+ return 255;
+ }
+ return _mapping[index];
}
} // storage
diff --git a/storage/src/vespa/storage/storageserver/priorityconverter.h b/storage/src/vespa/storage/storageserver/priorityconverter.h
index 47326e54243..48c7424433b 100644
--- a/storage/src/vespa/storage/storageserver/priorityconverter.h
+++ b/storage/src/vespa/storage/storageserver/priorityconverter.h
@@ -2,50 +2,34 @@
#pragma once
-#include <vespa/storage/config/config-stor-prioritymapping.h>
-#include <vespa/config/helper/ifetchercallback.h>
#include <vespa/documentapi/messagebus/priority.h>
-#include <atomic>
#include <array>
-#include <mutex>
-
-namespace config {
- class ConfigUri;
- class ConfigFetcher;
-}
+#include <vector>
namespace storage {
-class PriorityConverter
- : public config::IFetcherCallback<
- vespa::config::content::core::StorPrioritymappingConfig>
-{
+class PriorityConverter {
public:
- using Config = vespa::config::content::core::StorPrioritymappingConfig;
-
- explicit PriorityConverter(const config::ConfigUri& configUri);
- ~PriorityConverter() override;
+ PriorityConverter();
+ ~PriorityConverter();
/** Converts the given priority into a storage api priority number. */
- uint8_t toStoragePriority(documentapi::Priority::Value) const;
+ [[nodiscard]] uint8_t toStoragePriority(documentapi::Priority::Value) const noexcept;
/** Converts the given priority into a document api priority number. */
- documentapi::Priority::Value toDocumentPriority(uint8_t) const;
-
- void configure(std::unique_ptr<Config> config) override;
+ [[nodiscard]] documentapi::Priority::Value toDocumentPriority(uint8_t storage_priority) const noexcept {
+ return _reverse_mapping[storage_priority];
+ }
private:
- static_assert(documentapi::Priority::PRI_ENUM_SIZE == 16,
- "Unexpected size of priority enumeration");
- static_assert(documentapi::Priority::PRI_LOWEST == 15,
- "Priority enum value out of bounds");
- static constexpr size_t PRI_ENUM_SIZE = documentapi::Priority::PRI_ENUM_SIZE;
+ void init_static_priority_mappings();
- std::array<std::atomic<uint8_t>, PRI_ENUM_SIZE> _mapping;
- std::map<uint8_t, documentapi::Priority::Value> _reverseMapping;
- mutable std::mutex _mutex;
+ static_assert(documentapi::Priority::PRI_ENUM_SIZE == 16, "Unexpected size of priority enumeration");
+ static_assert(documentapi::Priority::PRI_LOWEST == 15, "Priority enum value out of bounds");
+ static constexpr size_t PRI_ENUM_SIZE = documentapi::Priority::PRI_ENUM_SIZE;
- std::unique_ptr<config::ConfigFetcher> _configFetcher;
+ std::array<uint8_t, PRI_ENUM_SIZE> _mapping;
+ std::vector<documentapi::Priority::Value> _reverse_mapping;
};
} // storage