diff options
7 files changed, 58 insertions, 10 deletions
diff --git a/storage/src/tests/common/bucket_stripe_utils_test.cpp b/storage/src/tests/common/bucket_stripe_utils_test.cpp index ae9f656e4d7..a654c4fe83e 100644 --- a/storage/src/tests/common/bucket_stripe_utils_test.cpp +++ b/storage/src/tests/common/bucket_stripe_utils_test.cpp @@ -7,6 +7,7 @@ using document::BucketId; using storage::calc_num_stripe_bits; using storage::stripe_of_bucket_key; +using storage::adjusted_num_stripes; constexpr uint8_t MUB = storage::spi::BucketLimits::MinUsedBits; TEST(BucketStripeUtilsTest, stripe_of_bucket_key) @@ -29,6 +30,17 @@ TEST(BucketStripeUtilsTest, calc_num_stripe_bits) EXPECT_EQ(8, calc_num_stripe_bits(256)); } +TEST(BucketStripeUtilsTest, adjusted_num_stripes) +{ + EXPECT_EQ(0, adjusted_num_stripes(0)); + EXPECT_EQ(1, adjusted_num_stripes(1)); + EXPECT_EQ(2, adjusted_num_stripes(2)); + EXPECT_EQ(4, adjusted_num_stripes(3)); + EXPECT_EQ(256, adjusted_num_stripes(255)); + EXPECT_EQ(256, adjusted_num_stripes(256)); + EXPECT_EQ(256, adjusted_num_stripes(257)); +} + TEST(BucketStripeUtilsTest, max_stripe_values) { EXPECT_EQ(8, storage::MaxStripeBits); diff --git a/storage/src/tests/distributor/distributortest.cpp b/storage/src/tests/distributor/distributortest.cpp index 660d58a9a28..3d1c6165946 100644 --- a/storage/src/tests/distributor/distributortest.cpp +++ b/storage/src/tests/distributor/distributortest.cpp @@ -1255,7 +1255,7 @@ TEST_F(DistributorTest, wanted_split_bit_count_is_lower_bounded) { } TEST_F(DistributorTest, host_info_sent_immediately_once_all_stripes_first_reported) { - set_num_distributor_stripes(3); + set_num_distributor_stripes(4); createLinks(); getClock().setAbsoluteTimeInSeconds(1000); // TODO STRIPE can't call this currently since it touches the bucket DB updater directly: @@ -1265,6 +1265,7 @@ TEST_F(DistributorTest, host_info_sent_immediately_once_all_stripes_first_report EXPECT_EQ(0, explicit_node_state_reply_send_invocations()); // Nothing yet getDistributor().notify_stripe_wants_to_send_host_info(1); getDistributor().notify_stripe_wants_to_send_host_info(2); + getDistributor().notify_stripe_wants_to_send_host_info(3); tickDistributorNTimes(1); // Still nothing. Missing initial report from stripe 0 diff --git a/storage/src/vespa/storage/common/bucket_stripe_utils.cpp b/storage/src/vespa/storage/common/bucket_stripe_utils.cpp index f1454403153..10667e79678 100644 --- a/storage/src/vespa/storage/common/bucket_stripe_utils.cpp +++ b/storage/src/vespa/storage/common/bucket_stripe_utils.cpp @@ -26,18 +26,28 @@ stripe_of_bucket_key(uint64_t key, uint8_t n_stripe_bits) noexcept } uint8_t -calc_num_stripe_bits(size_t n_stripes) noexcept +calc_num_stripe_bits(uint32_t n_stripes) noexcept { assert(n_stripes > 0); if (n_stripes == 1) { return 0; } - assert(n_stripes <= MaxStripes); - assert(n_stripes == vespalib::roundUp2inN(n_stripes)); + assert(n_stripes == adjusted_num_stripes(n_stripes)); auto result = vespalib::Optimized::msbIdx(n_stripes); assert(result <= MaxStripeBits); return result; } +uint32_t adjusted_num_stripes(uint32_t n_stripes) noexcept +{ + if (n_stripes > 1) { + if (n_stripes > MaxStripes) { + return MaxStripes; + } + return vespalib::roundUp2inN(n_stripes); + } + return n_stripes; +} + } diff --git a/storage/src/vespa/storage/common/bucket_stripe_utils.h b/storage/src/vespa/storage/common/bucket_stripe_utils.h index b4052a893a2..96f1247a09f 100644 --- a/storage/src/vespa/storage/common/bucket_stripe_utils.h +++ b/storage/src/vespa/storage/common/bucket_stripe_utils.h @@ -9,7 +9,7 @@ namespace storage { constexpr static uint8_t MaxStripeBits = spi::BucketLimits::MinUsedBits; -constexpr static size_t MaxStripes = (1ULL << MaxStripeBits); +constexpr static uint32_t MaxStripes = (1ULL << MaxStripeBits); /** * Returns the stripe in which the given bucket key belongs, @@ -22,7 +22,14 @@ size_t stripe_of_bucket_key(uint64_t key, uint8_t n_stripe_bits) noexcept; * * This also asserts that the number of stripes is valid (power of 2 and within MaxStripes boundary). */ -uint8_t calc_num_stripe_bits(size_t n_stripes) noexcept; +uint8_t calc_num_stripe_bits(uint32_t n_stripes) noexcept; + +/** + * Validates the number of stripes and returns the (potentially adjusted) value. + * + * This ensures the number of stripes is a power of 2 and within MaxStripes boundary. + */ +[[nodiscard]] uint32_t adjusted_num_stripes(uint32_t n_stripes) noexcept; } diff --git a/storage/src/vespa/storage/distributor/distributor.cpp b/storage/src/vespa/storage/distributor/distributor.cpp index 47f7fee5873..d85a4ff41d1 100644 --- a/storage/src/vespa/storage/distributor/distributor.cpp +++ b/storage/src/vespa/storage/distributor/distributor.cpp @@ -16,6 +16,7 @@ #include "ownership_transfer_safe_time_point_calculator.h" #include "throttlingoperationstarter.h" #include <vespa/document/bucket/fixed_bucket_spaces.h> +#include <vespa/storage/common/bucket_stripe_utils.h> #include <vespa/storage/common/global_bucket_space_distribution_converter.h> #include <vespa/storage/common/hostreporter/hostinfo.h> #include <vespa/storage/common/node_identity.h> @@ -88,6 +89,7 @@ Distributor::Distributor(DistributorComponentRegister& compReg, _component.registerMetric(*_metrics); _component.registerMetricUpdateHook(_metricUpdateHook, framework::SecondTime(0)); if (!_use_legacy_mode) { + assert(num_distributor_stripes == adjusted_num_stripes(num_distributor_stripes)); LOG(info, "Setting up distributor with %u stripes", num_distributor_stripes); // TODO STRIPE remove once legacy gone _stripe_accessor = std::make_unique<MultiThreadedStripeAccessor>(_stripe_pool); _bucket_db_updater = std::make_unique<BucketDBUpdater>(_component, _component, diff --git a/storage/src/vespa/storage/distributor/distributor_stripe_pool.cpp b/storage/src/vespa/storage/distributor/distributor_stripe_pool.cpp index b7183f4e157..13162de4208 100644 --- a/storage/src/vespa/storage/distributor/distributor_stripe_pool.cpp +++ b/storage/src/vespa/storage/distributor/distributor_stripe_pool.cpp @@ -77,7 +77,7 @@ void DistributorStripePool::park_thread_until_released(DistributorStripeThread& void DistributorStripePool::start(const std::vector<TickableStripe*>& stripes) { assert(!stripes.empty()); assert(_stripes.empty() && _threads.empty()); - // Note: This also asserts that the number of stripes is valid (power of 2 and within MaxStripes boundary). + assert(stripes.size() == adjusted_num_stripes(stripes.size())); _n_stripe_bits = calc_num_stripe_bits(stripes.size()); _stripes.reserve(stripes.size()); _threads.reserve(stripes.size()); diff --git a/storageserver/src/vespa/storageserver/app/distributorprocess.cpp b/storageserver/src/vespa/storageserver/app/distributorprocess.cpp index ede7fd1c9c0..45802df8866 100644 --- a/storageserver/src/vespa/storageserver/app/distributorprocess.cpp +++ b/storageserver/src/vespa/storageserver/app/distributorprocess.cpp @@ -1,9 +1,10 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "distributorprocess.h" -#include <vespa/storage/common/storagelink.h> -#include <vespa/storage/common/i_storage_chain_builder.h> #include <vespa/config/helper/configgetter.hpp> +#include <vespa/storage/common/bucket_stripe_utils.h> +#include <vespa/storage/common/i_storage_chain_builder.h> +#include <vespa/storage/common/storagelink.h> #include <vespa/log/log.h> LOG_SETUP(".process.distributor"); @@ -32,6 +33,21 @@ DistributorProcess::shutdown() _node.reset(); } +namespace { + +uint32_t +adjusted_num_distributor_stripes(uint32_t cfg_n_stripes) +{ + uint32_t adjusted_n_stripes = storage::adjusted_num_stripes(cfg_n_stripes); + if (adjusted_n_stripes != cfg_n_stripes) { + LOG(warning, "Configured number of distributor stripes (%u) is not valid. Adjusting to a valid value (%u)", + cfg_n_stripes, adjusted_n_stripes); + } + return adjusted_n_stripes; +} + +} + void DistributorProcess::setupConfig(milliseconds subscribeTimeout) { @@ -40,7 +56,7 @@ DistributorProcess::setupConfig(milliseconds subscribeTimeout) auto distr_cfg = config::ConfigGetter<StorDistributormanagerConfig>::getConfig( _configUri.getConfigId(), _configUri.getContext(), subscribeTimeout); - _num_distributor_stripes = distr_cfg->numDistributorStripes; + _num_distributor_stripes = adjusted_num_distributor_stripes(distr_cfg->numDistributorStripes); _distributorConfigHandler = _configSubscriber.subscribe<StorDistributormanagerConfig>(_configUri.getConfigId(), subscribeTimeout); _visitDispatcherConfigHandler = _configSubscriber.subscribe<StorVisitordispatcherConfig>(_configUri.getConfigId(), subscribeTimeout); Process::setupConfig(subscribeTimeout); |