Address low-hanging TODO fruit and remove stuff that's either done or won't be done

11 files changed, 20 insertions, 33 deletions

diff --git a/storage/src/tests/distributor/distributor_stripe_test.cpp b/storage/src/tests/distributor/distributor_stripe_test.cpp
index a61ea61854e..418b224eb25 100644
--- a/storage/src/tests/distributor/distributor_stripe_test.cpp
+++ b/storage/src/tests/distributor/distributor_stripe_test.cpp
@@ -230,7 +230,6 @@ TEST_F(DistributorStripeTest, operations_generated_and_started_without_duplicate
     ASSERT_EQ(6, _sender.commands().size());
 }
 
-// TODO STRIPE also need to impl/test cross-stripe cluster state changes
 TEST_F(DistributorStripeTest, recovery_mode_on_cluster_state_change)
 {
     setup_stripe(Redundancy(1), NodeCount(2),
@@ -333,7 +332,6 @@ TEST_F(DistributorStripeTest, update_bucket_database)
               updateBucketDB("0:456", "2:333", ResetTrusted(true)));
 }
 
-// TODO STRIPE need to impl/test cross-stripe config propagation
 TEST_F(DistributorStripeTest, priority_config_is_propagated_to_distributor_configuration)
 {
     using namespace vespa::config::content::core;
@@ -734,8 +732,6 @@ void assert_invalid_stats_for_all_spaces(
 
 }
 
-// TODO STRIPE must impl/test cross-stripe bucket space stats
-// TODO STRIPE cross-stripe recovery mode handling how?
 TEST_F(DistributorStripeTest, entering_recovery_mode_resets_bucket_space_stats)
 {
     // Set up a cluster state + DB contents which implies merge maintenance ops
diff --git a/storage/src/tests/distributor/legacy_bucket_db_updater_test.cpp b/storage/src/tests/distributor/legacy_bucket_db_updater_test.cpp
index b871bf5841e..5fa3ae5840b 100644
--- a/storage/src/tests/distributor/legacy_bucket_db_updater_test.cpp
+++ b/storage/src/tests/distributor/legacy_bucket_db_updater_test.cpp
@@ -57,7 +57,6 @@ getRequestBucketInfoStrings(uint32_t count)
 
 }
 
-// TODO STRIPE: Add variant of this test for the new stripe mode.
 // TODO STRIPE: Remove this test when legacy mode is gone.
 class LegacyBucketDBUpdaterTest : public Test,
                                   public DistributorTestUtil
diff --git a/storage/src/tests/distributor/legacy_distributor_test.cpp b/storage/src/tests/distributor/legacy_distributor_test.cpp
index 0a472430e78..90d64ddb130 100644
--- a/storage/src/tests/distributor/legacy_distributor_test.cpp
+++ b/storage/src/tests/distributor/legacy_distributor_test.cpp
@@ -33,7 +33,6 @@ using namespace ::testing;
 
 namespace storage::distributor {
 
-// TODO STRIPE: Add variant of this test for the new stripe mode.
 // TODO STRIPE: Remove this test when legacy mode is gone.
 struct LegacyDistributorTest : Test, DistributorTestUtil {
     LegacyDistributorTest();
@@ -65,7 +64,7 @@ struct LegacyDistributorTest : Test, DistributorTestUtil {
 
     void configureDistributor(const ConfigBuilder& config) {
         getConfig().configure(config);
-        _distributor->enableNextConfig();
+        _distributor->enable_next_config_if_changed();
     }
 
     auto currentReplicaCountingMode() const noexcept {
@@ -774,7 +773,7 @@ LegacyDistributorTest::configureMaxClusterClockSkew(int seconds) {
     ConfigBuilder builder;
     builder.maxClusterClockSkewSec = seconds;
     getConfig().configure(builder);
-    _distributor->enableNextConfig();
+    _distributor->enable_next_config_if_changed();
 }
 
 // Migrated to DistributorStripeTest
@@ -866,7 +865,7 @@ void LegacyDistributorTest::configure_mutation_sequencing(bool enabled) {
     ConfigBuilder builder;
     builder.sequenceMutatingOperations = enabled;
     getConfig().configure(builder);
-    _distributor->enableNextConfig();
+    _distributor->enable_next_config_if_changed();
 }
 
 // Migrated to DistributorStripeTest
@@ -892,7 +891,7 @@ LegacyDistributorTest::configure_merge_busy_inhibit_duration(int seconds) {
     ConfigBuilder builder;
     builder.inhibitMergeSendingOnBusyNodeDurationSec = seconds;
     getConfig().configure(builder);
-    _distributor->enableNextConfig();
+    _distributor->enable_next_config_if_changed();
 }
 
 // Migrated to DistributorStripeTest
diff --git a/storage/src/tests/distributor/top_level_distributor_test.cpp b/storage/src/tests/distributor/top_level_distributor_test.cpp
index eb2f8217872..e1bd82ffb3e 100644
--- a/storage/src/tests/distributor/top_level_distributor_test.cpp
+++ b/storage/src/tests/distributor/top_level_distributor_test.cpp
@@ -171,7 +171,7 @@ TEST_F(TopLevelDistributorTest, distributor_considered_initialized_once_self_obs
     EXPECT_TRUE(_distributor->done_initializing());
 }
 
-// TODO STRIPE consider moving to generic test, not specific to top-level distributor or stripe
+// TODO consider moving to generic test, not specific to top-level distributor or stripe
 TEST_F(TopLevelDistributorTest, contains_time_statement) {
     setup_distributor(Redundancy(1), NodeCount(1), "storage:1 distributor:1");
 
diff --git a/storage/src/vespa/storage/config/stor-distributormanager.def b/storage/src/vespa/storage/config/stor-distributormanager.def
index 887761ab3b5..2b3cec0e652 100644
--- a/storage/src/vespa/storage/config/stor-distributormanager.def
+++ b/storage/src/vespa/storage/config/stor-distributormanager.def
@@ -274,6 +274,9 @@ prioritize_global_bucket_merges bool default=true
 ## than 1.
 max_activation_inhibited_out_of_sync_groups int default=0
 
-## TODO STRIPE document
-## If 0, legacy single stripe behavior is used. Currently supports 0 or 1.
+## Specifies the number of stripes over which a distributor internally distributes
+## its buckets and operation processing. Every stripe receives its own thread.
+## If <= 0, the number of stripes is inferred automatically based on the number of
+## CPU cores available. If > 0, the number of stripes is explicitly overridden.
+## Stripe counts must be a power of two.
 num_distributor_stripes int default=0 restart
diff --git a/storage/src/vespa/storage/distributor/distributor_stripe.cpp b/storage/src/vespa/storage/distributor/distributor_stripe.cpp
index d9381c4abf0..1751b05b25d 100644
--- a/storage/src/vespa/storage/distributor/distributor_stripe.cpp
+++ b/storage/src/vespa/storage/distributor/distributor_stripe.cpp
@@ -28,12 +28,6 @@ using namespace std::chrono_literals;
 
 namespace storage::distributor {
 
-/* TODO STRIPE
- *  - need a DistributorStripeComponent per stripe
- *    - or better, remove entirely!
- *    - probably also DistributorStripeInterface since it's used to send
- *  - metrics aggregation
- */
 DistributorStripe::DistributorStripe(DistributorComponentRegister& compReg,
                                      DistributorMetricSet& metrics,
                                      IdealStateMetricSet& ideal_state_metrics,
diff --git a/storage/src/vespa/storage/distributor/distributor_stripe.h b/storage/src/vespa/storage/distributor/distributor_stripe.h
index 6a3f39d7576..d584960726a 100644
--- a/storage/src/vespa/storage/distributor/distributor_stripe.h
+++ b/storage/src/vespa/storage/distributor/distributor_stripe.h
@@ -45,7 +45,11 @@ class StripeHostInfoNotifier;
 class ThrottlingOperationStarter;
 
 /**
- * TODO STRIPE add class comment.
+ * A DistributorStripe encapsulates client operation handling and maintenance of a subset of the
+ * bucket space that the full distributor has responsibility for.
+ *
+ * Each distributor stripe is responsible for a completely disjoint subset of the bucket space of all
+ * other distributor stripes in the process (and transitively, in the entire cluster).
  */
 class DistributorStripe final
     : public DistributorStripeInterface,
diff --git a/storage/src/vespa/storage/distributor/tickable_stripe.h b/storage/src/vespa/storage/distributor/tickable_stripe.h
index ba74184313b..5b263ffe034 100644
--- a/storage/src/vespa/storage/distributor/tickable_stripe.h
+++ b/storage/src/vespa/storage/distributor/tickable_stripe.h
@@ -1,7 +1,7 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #pragma once
 
-#include "stripe_access_guard.h" // TODO STRIPE break up
+#include "stripe_access_guard.h"
 
 namespace storage::lib {
 class ClusterState;
diff --git a/storage/src/vespa/storage/distributor/top_level_bucket_db_updater.cpp b/storage/src/vespa/storage/distributor/top_level_bucket_db_updater.cpp
index 087ad01d65c..191be2a6766 100644
--- a/storage/src/vespa/storage/distributor/top_level_bucket_db_updater.cpp
+++ b/storage/src/vespa/storage/distributor/top_level_bucket_db_updater.cpp
@@ -97,7 +97,6 @@ TopLevelBucketDBUpdater::propagate_distribution_config(const BucketSpaceDistribu
 void
 TopLevelBucketDBUpdater::flush()
 {
-    // TODO STRIPE: Consider if this must flush_and_close() all stripes
 }
 
 void
@@ -142,8 +141,6 @@ TopLevelBucketDBUpdater::remove_superfluous_buckets(
                 old_cluster_state.toString().c_str(), new_cluster_state->toString().c_str());
             continue;
         }
-        // TODO STRIPE should we also pass old state and distr config? Must ensure we're in sync with stripe...
-        //   .. but config is set synchronously via the guard upon pending state creation edge
         auto maybe_lost = guard.remove_superfluous_buckets(elem.first, *new_cluster_state, is_distribution_config_change);
         if (maybe_lost.buckets != 0) {
             LOGBM(info, "After cluster state change %s, %zu buckets no longer "
@@ -213,7 +210,7 @@ TopLevelBucketDBUpdater::storage_distribution_changed(const BucketSpaceDistribut
             std::move(clusterInfo),
             _sender,
             _op_ctx.bucket_space_repo(), // TODO STRIPE cannot use!
-            _op_ctx.generate_unique_timestamp()); // TODO STRIPE must ensure no stripes can generate < this
+            _op_ctx.generate_unique_timestamp());
     _outdated_nodes_map = _pending_cluster_state->getOutdatedNodesMap();
 
     guard->set_pending_cluster_state_bundle(_pending_cluster_state->getNewClusterStateBundle());
diff --git a/storage/src/vespa/storage/distributor/top_level_distributor.cpp b/storage/src/vespa/storage/distributor/top_level_distributor.cpp
index d99a02081d8..ae414b2a85e 100644
--- a/storage/src/vespa/storage/distributor/top_level_distributor.cpp
+++ b/storage/src/vespa/storage/distributor/top_level_distributor.cpp
@@ -410,8 +410,6 @@ TopLevelDistributor::stripe_of_bucket_id(const document::BucketId& bucket_id, co
 bool
 TopLevelDistributor::onDown(const std::shared_ptr<api::StorageMessage>& msg)
 {
-    // TODO STRIPE can we route both requests and responses that are BucketCommand|Reply based on their bucket alone?
-    //   that covers most operations already...
     if (_use_legacy_mode) {
         return _stripe->handle_or_enqueue_message(msg);
     } else {
@@ -434,12 +432,10 @@ TopLevelDistributor::onDown(const std::shared_ptr<api::StorageMessage>& msg)
 bool
 TopLevelDistributor::handleReply(const std::shared_ptr<api::StorageReply>& reply)
 {
-    // TODO STRIPE this is used by tests. Do we need to invoke TopLevelBucketDBUpdater for any of them?
     assert(_use_legacy_mode);
     return _stripe->handleReply(reply);
 }
 
-// TODO STRIPE we need to reintroduce the top-level message queue...
 bool
 TopLevelDistributor::handleMessage(const std::shared_ptr<api::StorageMessage>& msg)
 {
@@ -640,7 +636,7 @@ TopLevelDistributor::doCriticalTick(framework::ThreadIndex idx)
         fetch_external_messages();
     }
     // Propagates any new configs down to stripe(s)
-    enableNextConfig();
+    enable_next_config_if_changed();
     if (_use_legacy_mode) {
         _stripe->doCriticalTick(idx);
         _tickResult.merge(_stripe->_tickResult);
@@ -665,7 +661,7 @@ TopLevelDistributor::doNonCriticalTick(framework::ThreadIndex idx)
 }
 
 void
-TopLevelDistributor::enableNextConfig() // TODO STRIPE rename to enable_next_config_if_changed()?
+TopLevelDistributor::enable_next_config_if_changed()
 {
     // Only lazily trigger a config propagation and internal update if something has _actually changed_.
     if (_component.internal_config_generation() != _current_internal_config_generation) {
diff --git a/storage/src/vespa/storage/distributor/top_level_distributor.h b/storage/src/vespa/storage/distributor/top_level_distributor.h
index ece9a61dfa3..81a30accf01 100644
--- a/storage/src/vespa/storage/distributor/top_level_distributor.h
+++ b/storage/src/vespa/storage/distributor/top_level_distributor.h
@@ -186,7 +186,7 @@ private:
      */
     void propagateInternalScanMetricsToExternal();
     void scanAllBuckets();
-    void enableNextConfig();
+    void enable_next_config_if_changed();
     void fetch_status_requests();
     void handle_status_requests();
     void signal_work_was_done();
@@ -224,7 +224,6 @@ private:
     std::shared_ptr<IdealStateMetricSet>  _ideal_state_metrics;
     std::shared_ptr<IdealStateTotalMetrics> _ideal_state_total_metrics;
     ChainedMessageSender*                 _messageSender;
-    // TODO STRIPE multiple stripes...! This is for proof of concept of wiring.
     uint8_t                               _n_stripe_bits;
     std::unique_ptr<DistributorStripe>    _stripe;
     DistributorStripePool&                _stripe_pool;