Merge pull request #18050 from vespa-engine/geirst/validate-distributor-stripes-config

Add validation of the number of distributor stripes from config and a…

6 files changed, 39 insertions, 7 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());