From 4134de20183cd3bc84420e20142ef69a5fb0fcbf Mon Sep 17 00:00:00 2001 From: Geir Storli Date: Fri, 28 May 2021 13:31:41 +0000 Subject: Add validation of the number of distributor stripes from config and add more asserts. This ensures the number of stripes is a power of 2 and within MaxStripes boundary. --- storage/src/tests/common/bucket_stripe_utils_test.cpp | 12 ++++++++++++ storage/src/tests/distributor/distributortest.cpp | 3 ++- storage/src/vespa/storage/common/bucket_stripe_utils.cpp | 16 +++++++++++++--- storage/src/vespa/storage/common/bucket_stripe_utils.h | 11 +++++++++-- storage/src/vespa/storage/distributor/distributor.cpp | 2 ++ .../storage/distributor/distributor_stripe_pool.cpp | 2 +- 6 files changed, 39 insertions(+), 7 deletions(-) (limited to 'storage') 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 +#include #include #include #include @@ -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(_stripe_pool); _bucket_db_updater = std::make_unique(_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& 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()); -- cgit v1.2.3