diff options
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<lib::Distribution> default_grouped_distribution() { return std::make_unique<lib::Distribution>( - 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<lib::Distribution> 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> group = (isLeafGroup) ? std::make_unique<Group>(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<n; ++i) { - const NodeState& ns(cs.getNodeState(Node(NodeType::DISTRIBUTOR, g.getNodes()[i]))); + for (unsigned short node : g.getNodes()) { + const NodeState& ns(cs.getNodeState(Node(NodeType::DISTRIBUTOR, node))); if (ns.getState().oneOf("ui")) return false; } } else { @@ -378,38 +375,38 @@ Distribution::getIdealNodes(const NodeType& nodeType, const ClusterState& cluste ss << "There is no legal distributor target in state with version " << clusterState.getVersion(); throw NoDistributorsAvailableException(ss.str(), VESPA_STRLOC); } - _groupDistribution.push_back(ResultGroup(*group, 1)); + _groupDistribution.emplace_back(*group, 1); } RandomGen random(seed); uint32_t randomIndex = 0; std::vector<ScoredNode> tmpResults; - for (uint32_t i=0, n=_groupDistribution.size(); i<n; ++i) { - uint16_t groupRedundancy(_groupDistribution[i]._redundancy); - const std::vector<uint16_t>& nodes(_groupDistribution[i]._group->getNodes()); + for (const auto & group : _groupDistribution) { + uint16_t groupRedundancy(group._redundancy); + const std::vector<uint16_t>& 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<o; ++k) { + for (uint32_t k=randomIndex, o=node; k<o; ++k) { random.nextDouble(); } - randomIndex = nodes[j]; + randomIndex = node; } double score = random.nextDouble(); ++randomIndex; @@ -417,7 +414,7 @@ Distribution::getIdealNodes(const NodeType& nodeType, const ClusterState& cluste score = std::pow(score, 1.0 / nodeState.getCapacity().getValue()); } if (score > 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<DistributionConfig> cfg) noexcept; + explicit ConfigWrapper(std::unique_ptr<DistributionConfig> 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); } |