From 4e419ba6ab10d40b033ce1b9164e084f2ad5eea2 Mon Sep 17 00:00:00 2001 From: Tor Brede Vekterli Date: Thu, 16 Sep 2021 13:27:01 +0000 Subject: Address low-hanging TODO fruit and remove stuff that's either done or won't be done --- storage/src/tests/distributor/distributor_stripe_test.cpp | 4 ---- storage/src/tests/distributor/legacy_bucket_db_updater_test.cpp | 1 - storage/src/tests/distributor/legacy_distributor_test.cpp | 9 ++++----- storage/src/tests/distributor/top_level_distributor_test.cpp | 2 +- storage/src/vespa/storage/config/stor-distributormanager.def | 7 +++++-- storage/src/vespa/storage/distributor/distributor_stripe.cpp | 6 ------ storage/src/vespa/storage/distributor/distributor_stripe.h | 6 +++++- storage/src/vespa/storage/distributor/tickable_stripe.h | 2 +- .../vespa/storage/distributor/top_level_bucket_db_updater.cpp | 5 +---- storage/src/vespa/storage/distributor/top_level_distributor.cpp | 8 ++------ storage/src/vespa/storage/distributor/top_level_distributor.h | 3 +-- 11 files changed, 20 insertions(+), 33 deletions(-) (limited to 'storage') 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& 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& msg) bool TopLevelDistributor::handleReply(const std::shared_ptr& 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& 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 _ideal_state_metrics; std::shared_ptr _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 _stripe; DistributorStripePool& _stripe_pool; -- cgit v1.2.3