From 359bc341f0a85993a231c16f0e7b8c508ea55dc8 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Sat, 3 Feb 2024 18:47:00 +0000 Subject: GC unused disk_distribution config. --- configdefinitions/src/vespa/stor-distribution.def | 14 +------------- .../global_bucket_space_distribution_converter_test.cpp | 4 ---- .../common/global_bucket_space_distribution_converter.cpp | 8 +++----- 3 files changed, 4 insertions(+), 22 deletions(-) diff --git a/configdefinitions/src/vespa/stor-distribution.def b/configdefinitions/src/vespa/stor-distribution.def index fc73983b0b8..810df64cc42 100644 --- a/configdefinitions/src/vespa/stor-distribution.def +++ b/configdefinitions/src/vespa/stor-distribution.def @@ -36,7 +36,7 @@ active_per_leaf_group bool default=false ## these cases they use many small groups to be able to scale. In such cases, ## where groups are small, having distributors able to take over when none are ## available in a group is useful. -# TODO: Deprecated and unused, remove in Vespa 9 +# TODO: Deprecated and unused, remove in Vespa 9 GC and hardcode distributor_auto_ownership_transfer_on_whole_group_down bool default=true ## Hierarchical grouping divides the nodes into a tree of groups. Due to config @@ -58,15 +58,3 @@ group[].nodes[].index int # The system will migrate all data away from retired nodes such that they can # eventually be removed without partial data loss group[].nodes[].retired bool default=false - -## Which disk distribution to use. From the current choices, we very much -## recommend using MODULO_BID. The only reason to use any of the earlier one is -## during an upgrade where you dont want to mess up the distribution in case you -## want to revert the upgrade. -## -## MODULO old one (4.0) -## MODULO_INDEX with node index in seed -## MODULO_KNUTH with random(node index) in seed -## MODULO_BID using all used bits, except count bits, and random(node index) -## TODO: Deprecated and unused, remove in Vespa 9 -disk_distribution enum { MODULO, MODULO_INDEX, MODULO_KNUTH, MODULO_BID } default=MODULO_BID diff --git a/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp b/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp index 5631ec71e4d..61f62d86544 100644 --- a/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp +++ b/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp @@ -48,7 +48,6 @@ group[0].nodes[1].index 1 group[0].nodes[1].retired false group[0].nodes[2].index 2 group[0].nodes[2].retired false -disk_distribution MODULO_BID )"); } @@ -117,7 +116,6 @@ group[2].nodes[1].index 4 group[2].nodes[1].retired false group[2].nodes[2].index 5 group[2].nodes[2].retired false -disk_distribution MODULO_BID )"); EXPECT_EQ(expected_global_config, default_to_global_config(default_config)); } @@ -200,7 +198,6 @@ group[6].capacity 1 group[6].partitions "" group[6].nodes[0].index 3 group[6].nodes[0].retired false -disk_distribution MODULO_BID )"); EXPECT_EQ(expected_global_config, default_to_global_config(default_config)); } @@ -253,7 +250,6 @@ group[2].capacity 1 group[2].partitions "" group[2].nodes[0].index 2 group[2].nodes[0].retired false -disk_distribution MODULO_BID )"); EXPECT_EQ(expected_global_config, default_to_global_config(default_config)); } diff --git a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp index ec606af0690..f34c1f2d92a 100644 --- a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp +++ b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -22,8 +21,7 @@ struct Group { std::map> sub_groups; }; -void set_distribution_invariant_config_fields(DistributionConfigBuilder& builder, const DistributionConfig& source) { - builder.diskDistribution = source.diskDistribution; +void set_distribution_invariant_config_fields(DistributionConfigBuilder& builder) { builder.distributorAutoOwnershipTransferOnWholeGroupDown = true; builder.activePerLeafGroup = true; // TODO consider how to best support n-of-m replication for global docs @@ -155,14 +153,14 @@ void build_global_groups(DistributionConfigBuilder& builder, const DistributionC std::shared_ptr GlobalBucketSpaceDistributionConverter::convert_to_global(const DistributionConfig& source) { DistributionConfigBuilder builder; - set_distribution_invariant_config_fields(builder, source); + set_distribution_invariant_config_fields(builder); build_global_groups(builder, source); return std::make_shared(builder); } std::shared_ptr GlobalBucketSpaceDistributionConverter::convert_to_global(const lib::Distribution& distr) { - const auto src_config = distr.serialize(); + const auto & src_config = distr.serialize(); auto global_config = convert_to_global(*string_to_config(src_config)); return std::make_shared(*global_config); } -- cgit v1.2.3 From 3b590870d88f329c9219759d46924571ce3db66d Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Sat, 3 Feb 2024 19:18:50 +0000 Subject: GC unused distributor_auto_ownership_transfer_on_whole_group_down --- configdefinitions/src/vespa/stor-distribution.def | 8 ------ storage/src/tests/bucketdb/bucketmanagertest.cpp | 4 +-- ...al_bucket_space_distribution_converter_test.cpp | 4 --- .../top_level_bucket_db_updater_test.cpp | 12 -------- .../global_bucket_space_distribution_converter.cpp | 1 - .../pending_bucket_space_db_transition.cpp | 3 -- .../vdslib/distribution/DistributionTestCase.java | 2 +- .../src/vespa/vdslib/distribution/distribution.cpp | 33 ++++++++++------------ .../src/vespa/vdslib/distribution/distribution.h | 12 ++++---- 9 files changed, 23 insertions(+), 56 deletions(-) diff --git a/configdefinitions/src/vespa/stor-distribution.def b/configdefinitions/src/vespa/stor-distribution.def index 810df64cc42..5eee4b7256d 100644 --- a/configdefinitions/src/vespa/stor-distribution.def +++ b/configdefinitions/src/vespa/stor-distribution.def @@ -31,14 +31,6 @@ ready_copies int default=0 ## - That level distributes copies to all defined groups. active_per_leaf_group bool default=false -## Search have some unfortunate properties with some queries, adding a static -## query cost, independent of the number of documents searched on a node. For -## these cases they use many small groups to be able to scale. In such cases, -## where groups are small, having distributors able to take over when none are -## available in a group is useful. -# TODO: Deprecated and unused, remove in Vespa 9 GC and hardcode -distributor_auto_ownership_transfer_on_whole_group_down bool default=true - ## Hierarchical grouping divides the nodes into a tree of groups. Due to config ## liking flat structures. The tree of groups is represented by a single array ## of groups defined here, where index is a string that can have a form like diff --git a/storage/src/tests/bucketdb/bucketmanagertest.cpp b/storage/src/tests/bucketdb/bucketmanagertest.cpp index 45d8fab7061..6173a43e25e 100644 --- a/storage/src/tests/bucketdb/bucketmanagertest.cpp +++ b/storage/src/tests/bucketdb/bucketmanagertest.cpp @@ -689,7 +689,7 @@ public: static std::unique_ptr default_grouped_distribution() { return std::make_unique( - GlobalBucketSpaceDistributionConverter::string_to_config(vespalib::string( + lib::Distribution::ConfigWrapper(GlobalBucketSpaceDistributionConverter::string_to_config(vespalib::string( R"(redundancy 2 group[3] group[0].name "invalid" @@ -708,7 +708,7 @@ group[2].nodes[3] group[2].nodes[0].index 3 group[2].nodes[1].index 4 group[2].nodes[2].index 5 -)"))); +)")))); } static std::shared_ptr derived_global_grouped_distribution() { diff --git a/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp b/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp index 61f62d86544..774f90821fa 100644 --- a/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp +++ b/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp @@ -37,7 +37,6 @@ initial_redundancy 0 ensure_primary_persisted true ready_copies 3 active_per_leaf_group true -distributor_auto_ownership_transfer_on_whole_group_down true group[0].index "invalid" group[0].name "invalid" group[0].capacity 1 @@ -91,7 +90,6 @@ initial_redundancy 0 ensure_primary_persisted true ready_copies 6 active_per_leaf_group true -distributor_auto_ownership_transfer_on_whole_group_down true group[0].index "invalid" group[0].name "invalid" group[0].capacity 1 @@ -161,7 +159,6 @@ initial_redundancy 0 ensure_primary_persisted true ready_copies 4 active_per_leaf_group true -distributor_auto_ownership_transfer_on_whole_group_down true group[0].index "invalid" group[0].name "invalid" group[0].capacity 1 @@ -231,7 +228,6 @@ initial_redundancy 0 ensure_primary_persisted true ready_copies 3 active_per_leaf_group true -distributor_auto_ownership_transfer_on_whole_group_down true group[0].index "invalid" group[0].name "invalid" group[0].capacity 1 diff --git a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp index d21ecc814a5..51c0a75e45d 100644 --- a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp +++ b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp @@ -1580,18 +1580,6 @@ TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_with_group_down) { "distributor:6 .2.s:d storage:6")); } -TEST_F(TopLevelBucketDBUpdaterTest, pending_cluster_state_with_group_down_and_no_handover) { - std::string config = dist_config_6_nodes_across_4_groups(); - config += "distributor_auto_ownership_transfer_on_whole_group_down false\n"; - set_distribution(config); - - // Group is down, but config says to not do anything about it. - EXPECT_EQ(get_node_list({0, 1, 2, 3, 4, 5}, _bucket_spaces.size() - 1), - get_sent_nodes("distributor:6 storage:6", - "distributor:6 .2.s:d .3.s:d storage:6")); -} - - namespace { void diff --git a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp index f34c1f2d92a..1d632c14cd5 100644 --- a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp +++ b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp @@ -22,7 +22,6 @@ struct Group { }; void set_distribution_invariant_config_fields(DistributionConfigBuilder& builder) { - builder.distributorAutoOwnershipTransferOnWholeGroupDown = true; builder.activePerLeafGroup = true; // TODO consider how to best support n-of-m replication for global docs builder.ensurePrimaryPersisted = true; diff --git a/storage/src/vespa/storage/distributor/pending_bucket_space_db_transition.cpp b/storage/src/vespa/storage/distributor/pending_bucket_space_db_transition.cpp index aac16f8b618..19d66f629c5 100644 --- a/storage/src/vespa/storage/distributor/pending_bucket_space_db_transition.cpp +++ b/storage/src/vespa/storage/distributor/pending_bucket_space_db_transition.cpp @@ -303,9 +303,6 @@ PendingBucketSpaceDbTransition::nodeNeedsOwnershipTransferFromGroupDown( const lib::ClusterState& state) const { const auto &dist(_bucket_space_state.get_distribution()); - if (!dist.distributorAutoOwnershipTransferOnWholeGroupDown()) { - return false; // Not doing anything for downed groups. - } const lib::Group* group(dist.getNodeGraph().getGroupForNode(nodeIndex)); // If there is no group information associated with the node (because the // group has changed or the node has been removed from config), we must diff --git a/vdslib/src/test/java/com/yahoo/vdslib/distribution/DistributionTestCase.java b/vdslib/src/test/java/com/yahoo/vdslib/distribution/DistributionTestCase.java index b2afe2146fc..6fe36c2d259 100644 --- a/vdslib/src/test/java/com/yahoo/vdslib/distribution/DistributionTestCase.java +++ b/vdslib/src/test/java/com/yahoo/vdslib/distribution/DistributionTestCase.java @@ -337,7 +337,7 @@ public class DistributionTestCase { @Test public void testDistributorGroupTakeover() throws Exception { test = new DistributionTestFactory("hierarchical-grouping-distributor-takeover") - .setDistribution(buildHierarchicalConfig(6, 3, 1, "1|2|*", 3).distributor_auto_ownership_transfer_on_whole_group_down(true)) + .setDistribution(buildHierarchicalConfig(6, 3, 1, "1|2|*", 3)) .setNodeType(NodeType.DISTRIBUTOR) .setClusterState(new ClusterState("distributor:2 storage:9")); for (BucketId bucket : getTestBuckets()) { diff --git a/vdslib/src/vespa/vdslib/distribution/distribution.cpp b/vdslib/src/vespa/vdslib/distribution/distribution.cpp index 4d18c9e5ef3..7ca85998d7e 100644 --- a/vdslib/src/vespa/vdslib/distribution/distribution.cpp +++ b/vdslib/src/vespa/vdslib/distribution/distribution.cpp @@ -121,7 +121,7 @@ Distribution::configure(const vespa::config::content::StorDistributionConfig& co if (nodeGraph) { path = DistributionConfigUtil::getGroupPath(cg.index); } - bool isLeafGroup = (cg.nodes.size() > 0); + bool isLeafGroup = ! cg.nodes.empty(); uint16_t index = (path.empty() ? 0 : path.back()); std::unique_ptr group = (isLeafGroup) ? std::make_unique(index, cg.name) @@ -163,7 +163,6 @@ Distribution::configure(const vespa::config::content::StorDistributionConfig& co _ensurePrimaryPersisted = config.ensurePrimaryPersisted; _readyCopies = config.readyCopies; _activePerGroup = config.activePerLeafGroup; - _distributorAutoOwnershipTransferOnWholeGroupDown = config.distributorAutoOwnershipTransferOnWholeGroupDown; } uint32_t @@ -318,9 +317,7 @@ Distribution::getIdealDistributorGroup(const document::BucketId& bucket, const C score = std::pow(score, 1.0 / subGroup.second->getCapacity().getValue()); } if (score > result._score) { - if (!_distributorAutoOwnershipTransferOnWholeGroupDown - || !allDistributorsDown(*subGroup.second, clusterState)) - { + if (!allDistributorsDown(*subGroup.second, clusterState)) { result = ScoredGroup(score, subGroup.second); } } @@ -335,8 +332,8 @@ bool Distribution::allDistributorsDown(const Group& g, const ClusterState& cs) { if (g.isLeafGroup()) { - for (uint32_t i=0, n=g.getNodes().size(); i tmpResults; - for (uint32_t i=0, n=_groupDistribution.size(); i& nodes(_groupDistribution[i]._group->getNodes()); + for (const auto & group : _groupDistribution) { + uint16_t groupRedundancy(group._redundancy); + const std::vector& nodes(group._group->getNodes()); // Create temporary place to hold results. // Stuff in redundancy fake entries to // avoid needing to check size during iteration. tmpResults.reserve(groupRedundancy); tmpResults.clear(); tmpResults.resize(groupRedundancy); - for (uint32_t j=0; j < nodes.size(); ++j) { + for (unsigned short node : nodes) { // Verify that the node is legal target before starting to grab // random number. Helps worst case of having to start new random // seed if the node that is out of order is illegal anyways. - const NodeState& nodeState(clusterState.getNodeState(Node(nodeType, nodes[j]))); + const NodeState& nodeState(clusterState.getNodeState(Node(nodeType, node))); if (!nodeState.getState().oneOf(upStates)) continue; // Get the score from the random number generator. Make sure we // pick correct random number. Optimize for the case where we // pick in rising order. - if (nodes[j] != randomIndex) { - if (nodes[j] < randomIndex) { + if (node != randomIndex) { + if (node < randomIndex) { random.setSeed(seed); randomIndex = 0; } - for (uint32_t k=randomIndex, o=nodes[j]; k tmpResults.back()._score) { - insertOrdered(tmpResults, ScoredNode(score, nodes[j])); + insertOrdered(tmpResults, ScoredNode(score, node)); } } trimResult(tmpResults, groupRedundancy); diff --git a/vdslib/src/vespa/vdslib/distribution/distribution.h b/vdslib/src/vespa/vdslib/distribution/distribution.h index ebe84ad3be9..38ddac30fc0 100644 --- a/vdslib/src/vespa/vdslib/distribution/distribution.h +++ b/vdslib/src/vespa/vdslib/distribution/distribution.h @@ -41,7 +41,6 @@ private: uint16_t _readyCopies; bool _activePerGroup; bool _ensurePrimaryPersisted; - bool _distributorAutoOwnershipTransferOnWholeGroupDown; vespalib::string _serialized; struct ResultGroup { @@ -91,7 +90,7 @@ public: public: ConfigWrapper(ConfigWrapper && rhs) noexcept = default; ConfigWrapper & operator = (ConfigWrapper && rhs) noexcept = default; - ConfigWrapper(std::unique_ptr cfg) noexcept; + explicit ConfigWrapper(std::unique_ptr cfg) noexcept; ~ConfigWrapper(); const DistributionConfig & get() const { return *_cfg; } private: @@ -99,10 +98,10 @@ public: }; Distribution(); Distribution(const Distribution&); - Distribution(const ConfigWrapper & cfg); - Distribution(const DistributionConfig & cfg); - Distribution(const vespalib::string& serialized); - ~Distribution(); + explicit Distribution(const ConfigWrapper & cfg); + explicit Distribution(const DistributionConfig & cfg); + explicit Distribution(const vespalib::string& serialized); + ~Distribution() override; Distribution& operator=(const Distribution&) = delete; @@ -113,7 +112,6 @@ public: uint16_t getInitialRedundancy() const noexcept { return _initialRedundancy; } uint16_t getReadyCopies() const noexcept { return _readyCopies; } bool ensurePrimaryPersisted() const noexcept { return _ensurePrimaryPersisted; } - bool distributorAutoOwnershipTransferOnWholeGroupDown() const noexcept { return _distributorAutoOwnershipTransferOnWholeGroupDown; } bool activePerGroup() const noexcept { return _activePerGroup; } bool operator==(const Distribution& o) const noexcept { return (_serialized == o._serialized); } -- cgit v1.2.3 From f68aab16a9e45b0341d458f7a6856e2348113a18 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Mon, 5 Feb 2024 10:53:01 +0000 Subject: Followup on review comments and initialize members explicit. --- .../common/global_bucket_space_distribution_converter.cpp | 2 +- vdslib/src/vespa/vdslib/distribution/distribution.cpp | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp index 1d632c14cd5..eb42f19a5e8 100644 --- a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp +++ b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp @@ -159,7 +159,7 @@ GlobalBucketSpaceDistributionConverter::convert_to_global(const DistributionConf std::shared_ptr GlobalBucketSpaceDistributionConverter::convert_to_global(const lib::Distribution& distr) { - const auto & src_config = distr.serialize(); + const auto src_config = distr.serialize(); auto global_config = convert_to_global(*string_to_config(src_config)); return std::make_shared(*global_config); } diff --git a/vdslib/src/vespa/vdslib/distribution/distribution.cpp b/vdslib/src/vespa/vdslib/distribution/distribution.cpp index 7ca85998d7e..4a174feffcc 100644 --- a/vdslib/src/vespa/vdslib/distribution/distribution.cpp +++ b/vdslib/src/vespa/vdslib/distribution/distribution.cpp @@ -44,6 +44,8 @@ Distribution::Distribution() _node2Group(), _redundancy(), _initialRedundancy(0), + _readyCopies(0), + _activePerGroup(false), _ensurePrimaryPersisted(true) { auto config(getDefaultDistributionConfig(0, 0)); @@ -60,6 +62,8 @@ Distribution::Distribution(const Distribution& d) _node2Group(), _redundancy(), _initialRedundancy(0), + _readyCopies(0), + _activePerGroup(false), _ensurePrimaryPersisted(true), _serialized(d._serialized) { @@ -84,6 +88,8 @@ Distribution::Distribution(const vespa::config::content::StorDistributionConfig _node2Group(), _redundancy(), _initialRedundancy(0), + _readyCopies(0), + _activePerGroup(false), _ensurePrimaryPersisted(true) { vespalib::asciistream ost; @@ -99,6 +105,8 @@ Distribution::Distribution(const vespalib::string& serialized) _node2Group(), _redundancy(), _initialRedundancy(0), + _readyCopies(0), + _activePerGroup(false), _ensurePrimaryPersisted(true), _serialized(serialized) { @@ -332,7 +340,7 @@ bool Distribution::allDistributorsDown(const Group& g, const ClusterState& cs) { if (g.isLeafGroup()) { - for (unsigned short node : g.getNodes()) { + for (uint16_t node : g.getNodes()) { const NodeState& ns(cs.getNodeState(Node(NodeType::DISTRIBUTOR, node))); if (ns.getState().oneOf("ui")) return false; } @@ -389,7 +397,7 @@ Distribution::getIdealNodes(const NodeType& nodeType, const ClusterState& cluste tmpResults.reserve(groupRedundancy); tmpResults.clear(); tmpResults.resize(groupRedundancy); - for (unsigned short node : nodes) { + for (uint16_t node : nodes) { // Verify that the node is legal target before starting to grab // random number. Helps worst case of having to start new random // seed if the node that is out of order is illegal anyways. -- cgit v1.2.3