From e7645af6648c572c2b4f095a5813a44efd098faf Mon Sep 17 00:00:00 2001 From: Brandon Allard Date: Fri, 31 May 2024 11:05:55 -0400 Subject: [PATCH 1/3] cluster/scheduling: use per shard indexes in even_topic_distributon_constraint --- .../scheduling/leader_balancer_constraints.cc | 63 +++++++++---------- .../scheduling/leader_balancer_constraints.h | 5 +- 2 files changed, 31 insertions(+), 37 deletions(-) diff --git a/src/v/cluster/scheduling/leader_balancer_constraints.cc b/src/v/cluster/scheduling/leader_balancer_constraints.cc index 521037c841646..419536567206d 100644 --- a/src/v/cluster/scheduling/leader_balancer_constraints.cc +++ b/src/v/cluster/scheduling/leader_balancer_constraints.cc @@ -41,9 +41,9 @@ void even_topic_distributon_constraint::update_index(const reassignment& r) { skew = adjusted_error(skew, topic_id, r.from, r.to); _error += skew; - // Update _topic_node_index - _topic_node_index.at(topic_id).at(r.from.node_id) -= 1; - _topic_node_index.at(topic_id).at(r.to.node_id) += 1; + // Update _topic_shard_index + _topic_shard_index.at(topic_id).at(r.from) -= 1; + _topic_shard_index.at(topic_id).at(r.to) += 1; // Update _si @@ -60,23 +60,22 @@ even_topic_distributon_constraint::recommended_reassignment() { } void even_topic_distributon_constraint::rebuild_indexes() { - _topic_node_index.clear(); + _topic_shard_index.clear(); _topic_replica_index.clear(); _topic_partition_index.clear(); for (const auto& broker_shard : si().shards()) { for (const auto& group_p : broker_shard.second) { auto topic_id = group_to_topic_id().at(group_p.first); - const auto& node_id = broker_shard.first.node_id; - _topic_node_index[topic_id][node_id] += 1; + _topic_shard_index[topic_id][broker_shard.first] += 1; _topic_partition_index[topic_id] += 1; // Some of the replicas may not have leadership. So add - // all replica nodes here. + // all replica shards here. for (const auto& replica_bs : group_p.second) { - _topic_replica_index[topic_id].insert(replica_bs.node_id); - _topic_node_index[topic_id].try_emplace(replica_bs.node_id); + _topic_replica_index[topic_id].insert(replica_bs); + _topic_shard_index[topic_id].try_emplace(replica_bs); } } } @@ -86,20 +85,20 @@ void even_topic_distributon_constraint::rebuild_indexes() { * Used to calculate the initial values for the error this constraint * is trying to minimize. The goal here is to calculate a per topic * error(or skew) where the error is zero if the leaders of a topic's - * partitions are evenly distributed on every node. And where the error + * partitions are evenly distributed on every shard. And where the error * grows to +infinity the more skewed the leadership assignment is to a - * subset of nodes. The equations used can be summarized as; + * subset of shards. The equations used can be summarized as; * - * skew[topic_i] = SUM(leaders[node_i, topic_i] - opt[topic_i])^2 - * opt[topic_i] = total_partitions[topic_i] / total_nodes[topic_i] - * where total_nodes is the number of nodes a topic has replicas on. + * skew[topic_i] = SUM(leaders[shard_i, topic_i] - opt[topic_i])^2 + * opt[topic_i] = total_partitions[topic_i] / total_shards[topic_i] + * where total_shards is the number of shards a topic has replicas on. * total_partitions is the number of partitions the topic has. */ void even_topic_distributon_constraint::calc_topic_skew() { _topic_skew.clear(); _topic_opt_leaders.clear(); - for (const auto& topic : _topic_node_index) { + for (const auto& topic : _topic_shard_index) { auto topic_partitions = static_cast( _topic_partition_index.at(topic.first)); auto topic_replicas = static_cast( @@ -111,8 +110,8 @@ void even_topic_distributon_constraint::calc_topic_skew() { auto& skew = _topic_skew[topic.first]; skew = 0; - for (const auto& node : topic.second) { - auto leaders = static_cast(node.second); + for (const auto& shard : topic.second) { + auto leaders = static_cast(shard.second); skew += pow(leaders - opt_leaders, 2); } @@ -127,39 +126,33 @@ double even_topic_distributon_constraint::adjusted_error( const topic_id_t& topic_id, const model::broker_shard& from, const model::broker_shard& to) const { - // Moving leadership between shards on the same node doesn't change - // error for this constraint. - if (from.node_id == to.node_id) { - return current_error; - } - auto opt_leaders = _topic_opt_leaders.at(topic_id); - const auto& topic_leaders = _topic_node_index.at(topic_id); + const auto& topic_leaders = _topic_shard_index.at(topic_id); - double from_node_leaders = 0; - const auto from_it = topic_leaders.find(from.node_id); + double from_shard_leaders = 0; + const auto from_it = topic_leaders.find(from); if (from_it != topic_leaders.cend()) { - from_node_leaders = static_cast(from_it->second); + from_shard_leaders = static_cast(from_it->second); } else { - // If there are no leaders for the topic on the from node + // If there are no leaders for the topic on the from shard // then there is nothing to move and no change to the error. return current_error; } - double to_node_leaders = 0; - const auto to_it = topic_leaders.find(to.node_id); + double to_shard_leaders = 0; + const auto to_it = topic_leaders.find(to); if (to_it != topic_leaders.cend()) { - to_node_leaders = static_cast(to_it->second); + to_shard_leaders = static_cast(to_it->second); } // Subtract old weights - current_error -= pow(from_node_leaders - opt_leaders, 2); - current_error -= pow(to_node_leaders - opt_leaders, 2); + current_error -= pow(from_shard_leaders - opt_leaders, 2); + current_error -= pow(to_shard_leaders - opt_leaders, 2); // Add new weights - current_error += pow((from_node_leaders - 1) - opt_leaders, 2); - current_error += pow((to_node_leaders + 1) - opt_leaders, 2); + current_error += pow((from_shard_leaders - 1) - opt_leaders, 2); + current_error += pow((to_shard_leaders + 1) - opt_leaders, 2); return current_error; } diff --git a/src/v/cluster/scheduling/leader_balancer_constraints.h b/src/v/cluster/scheduling/leader_balancer_constraints.h index 7daf0e02b0813..d100bc01fe6f4 100644 --- a/src/v/cluster/scheduling/leader_balancer_constraints.h +++ b/src/v/cluster/scheduling/leader_balancer_constraints.h @@ -158,9 +158,10 @@ class even_topic_distributon_constraint final double _error{0}; // Stores the number of leaders on a given node per topic. - topic_map> _topic_node_index; + topic_map> + _topic_shard_index; topic_map _topic_partition_index; - topic_map> _topic_replica_index; + topic_map> _topic_replica_index; topic_map _topic_skew; topic_map _topic_opt_leaders; From 882e8c84db1264f9751b5c0d234a8d62f564657e Mon Sep 17 00:00:00 2001 From: Brandon Allard Date: Fri, 31 May 2024 13:20:55 -0400 Subject: [PATCH 2/3] cluster/scheduling: use chunked_hash containers where possible --- .../scheduling/leader_balancer_constraints.cc | 8 +-- .../scheduling/leader_balancer_constraints.h | 24 ++++---- .../scheduling/leader_balancer_greedy.h | 7 ++- .../scheduling/leader_balancer_random.h | 9 ++- .../scheduling/leader_balancer_types.h | 11 ++-- src/v/cluster/tests/leader_balancer_bench.cc | 12 ++-- .../tests/leader_balancer_constraints_test.cc | 61 +++++++++++-------- src/v/cluster/tests/leader_balancer_test.cc | 12 +++- .../tests/leader_balancer_test_utils.h | 13 ++++ 9 files changed, 92 insertions(+), 65 deletions(-) diff --git a/src/v/cluster/scheduling/leader_balancer_constraints.cc b/src/v/cluster/scheduling/leader_balancer_constraints.cc index 419536567206d..5e4c0f3b0c397 100644 --- a/src/v/cluster/scheduling/leader_balancer_constraints.cc +++ b/src/v/cluster/scheduling/leader_balancer_constraints.cc @@ -17,9 +17,9 @@ namespace cluster::leader_balancer_types { even_topic_distributon_constraint::even_topic_distributon_constraint( group_id_to_topic_revision_t group_to_topic_rev, - shard_index si, + const shard_index& si, const muted_index& mi) - : _si(std::move(si)) + : _si(si) , _mi(mi) , _group_to_topic_rev(std::move(group_to_topic_rev)) { rebuild_indexes(); @@ -44,10 +44,6 @@ void even_topic_distributon_constraint::update_index(const reassignment& r) { // Update _topic_shard_index _topic_shard_index.at(topic_id).at(r.from) -= 1; _topic_shard_index.at(topic_id).at(r.to) += 1; - - // Update _si - - _si.update_index(r); } std::optional diff --git a/src/v/cluster/scheduling/leader_balancer_constraints.h b/src/v/cluster/scheduling/leader_balancer_constraints.h index d100bc01fe6f4..2ef8f4abcaac9 100644 --- a/src/v/cluster/scheduling/leader_balancer_constraints.h +++ b/src/v/cluster/scheduling/leader_balancer_constraints.h @@ -11,13 +11,13 @@ #pragma once #include "cluster/scheduling/leader_balancer_types.h" +#include "container/chunked_hash_map.h" #include "model/metadata.h" #include "raft/fundamental.h" #include #include -#include #include #include #include @@ -110,12 +110,12 @@ class even_topic_distributon_constraint final using topic_id_t = model::revision_id::type; template - using topic_map = absl::btree_map; + using topic_map = chunked_hash_map; public: even_topic_distributon_constraint( group_id_to_topic_revision_t group_to_topic_rev, - shard_index si, + const shard_index& si, const muted_index& mi); even_topic_distributon_constraint( @@ -152,20 +152,19 @@ class even_topic_distributon_constraint final std::optional recommended_reassignment() override; private: - shard_index _si; + std::reference_wrapper _si; std::reference_wrapper _mi; group_id_to_topic_revision_t _group_to_topic_rev; double _error{0}; // Stores the number of leaders on a given node per topic. - topic_map> - _topic_shard_index; + topic_map> _topic_shard_index; topic_map _topic_partition_index; - topic_map> _topic_replica_index; + topic_map> _topic_replica_index; topic_map _topic_skew; topic_map _topic_opt_leaders; - const shard_index& si() const { return _si; } + const shard_index& si() const { return _si.get(); } const muted_index& mi() const { return _mi.get(); } const group_id_to_topic_revision_t& group_to_topic_id() const { return _group_to_topic_rev; @@ -215,8 +214,8 @@ class even_shard_load_constraint final static constexpr double error_jitter = 0.000001; public: - even_shard_load_constraint(shard_index si, const muted_index& mi) - : _si(std::move(si)) + even_shard_load_constraint(const shard_index& si, const muted_index& mi) + : _si(si) , _mi(mi) , _num_cores(num_cores()) , _num_groups(num_groups()) { @@ -236,7 +235,6 @@ class even_shard_load_constraint final double error() const { return _error; } void update_index(const reassignment& r) override { _error = adjusted_error(_error, r.from, r.to); - _si.update_index(r); } /* @@ -261,13 +259,13 @@ class even_shard_load_constraint final double calc_target_load() const; private: - shard_index _si; + std::reference_wrapper _si; std::reference_wrapper _mi; size_t _num_cores; size_t _num_groups; double _error{0}; - const shard_index& si() const { return _si; } + const shard_index& si() const { return _si.get(); } const muted_index& mi() const { return _mi.get(); } /* diff --git a/src/v/cluster/scheduling/leader_balancer_greedy.h b/src/v/cluster/scheduling/leader_balancer_greedy.h index 530054840a74f..3d0a5276412de 100644 --- a/src/v/cluster/scheduling/leader_balancer_greedy.h +++ b/src/v/cluster/scheduling/leader_balancer_greedy.h @@ -34,8 +34,9 @@ class greedy_balanced_shards final : public leader_balancer_strategy { index_type cores, absl::flat_hash_set muted_nodes) : _mi(std::make_unique( std::move(muted_nodes), leader_balancer_types::muted_groups_t{})) - , _even_shard_load_c( - leader_balancer_types::shard_index{std::move(cores)}, *_mi) {} + , _si(std::make_unique( + std::move(cores))) + , _even_shard_load_c(*_si, *_mi) {} double error() const final { return _even_shard_load_c.error(); } @@ -62,6 +63,7 @@ class greedy_balanced_shards final : public leader_balancer_strategy { void apply_movement(const reassignment& r) final { _even_shard_load_c.update_index(r); + _si->update_index(r); } std::vector stats() const final { @@ -70,6 +72,7 @@ class greedy_balanced_shards final : public leader_balancer_strategy { private: std::unique_ptr _mi; + std::unique_ptr _si; leader_balancer_types::even_shard_load_constraint _even_shard_load_c; }; diff --git a/src/v/cluster/scheduling/leader_balancer_random.h b/src/v/cluster/scheduling/leader_balancer_random.h index bb243c1e9d5d0..3358725988031 100644 --- a/src/v/cluster/scheduling/leader_balancer_random.h +++ b/src/v/cluster/scheduling/leader_balancer_random.h @@ -104,9 +104,10 @@ class random_hill_climbing_strategy final : public leader_balancer_strategy { random_hill_climbing_strategy( index_type index, group_id_to_topic_revision_t g_to_ntp, muted_index mi) : _mi(std::make_unique(std::move(mi))) - , _reassignments(index) - , _etdc(std::move(g_to_ntp), shard_index(index), *_mi) - , _eslc(shard_index(std::move(index)), *_mi) {} + , _si(std::make_unique(std::move(index))) + , _reassignments(_si->shards()) + , _etdc(std::move(g_to_ntp), *_si, *_mi) + , _eslc(*_si, *_mi) {} double error() const override { return _eslc.error() + _etdc.error(); } @@ -147,6 +148,7 @@ class random_hill_climbing_strategy final : public leader_balancer_strategy { _etdc.update_index(reassignment); _eslc.update_index(reassignment); _mi->update_index(reassignment); + _si->update_index(reassignment); _reassignments.update_index(reassignment); } @@ -159,6 +161,7 @@ class random_hill_climbing_strategy final : public leader_balancer_strategy { static constexpr double error_jitter = 0.000001; std::unique_ptr _mi; + std::unique_ptr _si; random_reassignments _reassignments; even_topic_distributon_constraint _etdc; diff --git a/src/v/cluster/scheduling/leader_balancer_types.h b/src/v/cluster/scheduling/leader_balancer_types.h index ea385e6b16527..34672a92db541 100644 --- a/src/v/cluster/scheduling/leader_balancer_types.h +++ b/src/v/cluster/scheduling/leader_balancer_types.h @@ -10,13 +10,10 @@ */ #pragma once +#include "container/chunked_hash_map.h" #include "model/metadata.h" #include "raft/fundamental.h" -#include -#include -#include -#include #include namespace cluster::leader_balancer_types { @@ -37,12 +34,12 @@ struct reassignment { reassignment() = default; }; -using index_type = absl::node_hash_map< +using index_type = chunked_hash_map< model::broker_shard, - absl::btree_map>>; + chunked_hash_map>>; using group_id_to_topic_revision_t - = absl::btree_map; + = chunked_hash_map; using muted_groups_t = roaring::Roaring64Map; /* diff --git a/src/v/cluster/tests/leader_balancer_bench.cc b/src/v/cluster/tests/leader_balancer_bench.cc index 8943bff3f0a8a..5b0210cf93b26 100644 --- a/src/v/cluster/tests/leader_balancer_bench.cc +++ b/src/v/cluster/tests/leader_balancer_bench.cc @@ -53,7 +53,8 @@ void random_search_eval_bench(bool measure_all) { node_count, shards_per_node, groups_per_shard, replicas); cluster::leader_balancer_types::muted_index mi{{}, {}}; - cluster::leader_balancer_types::shard_index si(index); + cluster::leader_balancer_types::shard_index si( + leader_balancer_test_utils::copy_cluster_index(index)); auto gid_topic = make_gid_to_topic_index(si.shards()); if (measure_all) { @@ -62,9 +63,8 @@ void random_search_eval_bench(bool measure_all) { random_t rt{index}; cluster::leader_balancer_types::even_topic_distributon_constraint tdc( - gid_topic, si, mi); - cluster::leader_balancer_types::even_shard_load_constraint slc( - std::move(si), mi); + std::move(gid_topic), si, mi); + cluster::leader_balancer_types::even_shard_load_constraint slc(si, mi); if (!measure_all) { perf_tests::start_measuring_time(); @@ -104,7 +104,7 @@ void even_topic_c_bench(bool measure_all) { } cluster::leader_balancer_types::even_topic_distributon_constraint tdc( - gid_topic, si, mi); + std::move(gid_topic), si, mi); if (!measure_all) { perf_tests::start_measuring_time(); @@ -157,7 +157,7 @@ void balancer_bench(bool measure_all) { perf_tests::start_measuring_time(); } - auto greed = cluster::greedy_balanced_shards(index, {}); + auto greed = cluster::greedy_balanced_shards(std::move(index), {}); vassert(greed.error() == 0, ""); vassert(greed.error() == 0, "e > 0"); diff --git a/src/v/cluster/tests/leader_balancer_constraints_test.cc b/src/v/cluster/tests/leader_balancer_constraints_test.cc index 7f5a6a718fab2..98cdc3c4b2c9e 100644 --- a/src/v/cluster/tests/leader_balancer_constraints_test.cc +++ b/src/v/cluster/tests/leader_balancer_constraints_test.cc @@ -62,7 +62,8 @@ BOOST_AUTO_TEST_CASE(greedy_movement) { // 10 partitions per shard // r=3 (3 replicas) auto index = leader_balancer_test_utils::make_cluster_index(10, 2, 10, 3); - auto shard_index = lbt::shard_index(index); + auto shard_index = lbt::shard_index( + leader_balancer_test_utils::copy_cluster_index(index)); auto mute_index = lbt::muted_index({}, {}); auto greed = lbt::even_shard_load_constraint(shard_index, mute_index); @@ -74,14 +75,15 @@ BOOST_AUTO_TEST_CASE(greedy_movement) { index[shard20][raft::group_id(21)] = index[shard20][raft::group_id(3)]; index[shard20][raft::group_id(22)] = index[shard20][raft::group_id(3)]; - auto shard_index2 = lbt::shard_index(index); - auto greed2 = lbt::even_shard_load_constraint(shard_index2, mute_index); + shard_index = lbt::shard_index( + leader_balancer_test_utils::copy_cluster_index(index)); + auto greed2 = lbt::even_shard_load_constraint(shard_index, mute_index); BOOST_REQUIRE_GT(greed2.error(), 0); // movement should be _from_ the overloaded shard auto movement = greed2.recommended_reassignment(); BOOST_REQUIRE(movement); - check_valid(shard_index2.shards(), *movement); + check_valid(shard_index.shards(), *movement); BOOST_REQUIRE_EQUAL(movement->from, shard20); } @@ -216,7 +218,7 @@ BOOST_AUTO_TEST_CASE(even_topic_distribution_empty) { auto [shard_index, muted_index] = from_spec({}); auto even_topic_con = lbt::even_topic_distributon_constraint( - gntp_i, shard_index, muted_index); + std::move(gntp_i), shard_index, muted_index); BOOST_REQUIRE(even_topic_con.error() == 0); } @@ -239,7 +241,7 @@ BOOST_AUTO_TEST_CASE(even_topic_distribution_constraint_no_error) { auto mute_i = lbt::muted_index({}, {}); auto topic_constraint = lbt::even_topic_distributon_constraint( - g_id_to_t_id, index_cl, mute_i); + std::move(g_id_to_t_id), index_cl, mute_i); BOOST_REQUIRE(topic_constraint.error() == 0); } @@ -261,7 +263,7 @@ BOOST_AUTO_TEST_CASE(even_topic_distributon_constraint_uniform_move) { auto mute_i = lbt::muted_index({}, {}); auto topic_constraint = lbt::even_topic_distributon_constraint( - g_id_to_t_id, index_cl, mute_i); + std::move(g_id_to_t_id), index_cl, mute_i); auto reassignment = re(1, 0, 1); BOOST_REQUIRE(topic_constraint.error() != 0); @@ -295,7 +297,7 @@ BOOST_AUTO_TEST_CASE(even_topic_constraint_too_many_replicas) { auto mute_i = lbt::muted_index({}, {}); auto topic_constraint = lbt::even_topic_distributon_constraint( - g_id_to_t_id, index_cl, mute_i); + std::move(g_id_to_t_id), index_cl, mute_i); } BOOST_AUTO_TEST_CASE(even_topic_distributon_constraint_find_reassignment) { @@ -318,7 +320,7 @@ BOOST_AUTO_TEST_CASE(even_topic_distributon_constraint_find_reassignment) { auto mute_i = lbt::muted_index({}, {}); auto topic_constraint = lbt::even_topic_distributon_constraint( - g_id_to_t_id, index_cl, mute_i); + std::move(g_id_to_t_id), index_cl, mute_i); auto reassignment = re(1, 0, 1); BOOST_REQUIRE(topic_constraint.error() != 0); @@ -355,7 +357,7 @@ BOOST_AUTO_TEST_CASE(even_shard_no_error_even_topic_error) { auto even_shard_con = lbt::even_shard_load_constraint( shard_index, muted_index); auto even_topic_con = lbt::even_topic_distributon_constraint( - g_id_to_t_id, shard_index, muted_index); + std::move(g_id_to_t_id), shard_index, muted_index); BOOST_REQUIRE(even_shard_con.error() == 0); BOOST_REQUIRE(even_topic_con.error() > 0); @@ -392,7 +394,7 @@ BOOST_AUTO_TEST_CASE(even_topic_no_error_even_shard_error) { auto even_shard_con = lbt::even_shard_load_constraint( shard_index, muted_index); auto even_topic_con = lbt::even_topic_distributon_constraint( - g_id_to_t_id, shard_index, muted_index); + std::move(g_id_to_t_id), shard_index, muted_index); BOOST_REQUIRE(even_shard_con.error() > 0); BOOST_REQUIRE(even_shard_con.recommended_reassignment().has_value()); @@ -454,29 +456,37 @@ BOOST_AUTO_TEST_CASE(topic_skew_error) { // OMB testing. It ensures that the random hill climbing strategy // can properly balance this state. The minimum number of reassignments // needed to balance is 2. - auto g_id_to_t_id = group_to_topic_from_spec({ - {0, {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}}, - {1, {10, 11, 12, 13, 14}}, - }); - - auto [shard_index, muted_index] = from_spec( - { - {{0, 2, 10, 11, 12}, {13, 14, 1, 3, 4, 5, 6, 7, 8, 9}}, - {{1, 6, 8, 13, 14}, {10, 11, 12, 0, 2, 3, 4, 5, 7, 9}}, - {{3, 4, 5, 7, 9}, {10, 11, 12, 13, 14, 0, 1, 2, 6, 8}}, - }, - {}); + auto index_fn = [] { + return std::make_tuple( + from_spec( + { + {{0, 2, 10, 11, 12}, {13, 14, 1, 3, 4, 5, 6, 7, 8, 9}}, + {{1, 6, 8, 13, 14}, {10, 11, 12, 0, 2, 3, 4, 5, 7, 9}}, + {{3, 4, 5, 7, 9}, {10, 11, 12, 13, 14, 0, 1, 2, 6, 8}}, + }, + {}), + group_to_topic_from_spec({ + {0, {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}}, + {1, {10, 11, 12, 13, 14}}, + })); + }; + auto [o, g_id_to_t_id] = index_fn(); + auto [shard_index, muted_index] = std::move(o); auto even_shard_con = lbt::even_shard_load_constraint( shard_index, muted_index); auto even_topic_con = lbt::even_topic_distributon_constraint( - g_id_to_t_id, shard_index, muted_index); + std::move(g_id_to_t_id), shard_index, muted_index); BOOST_REQUIRE(even_shard_con.error() == 0); BOOST_REQUIRE(even_topic_con.error() > 0); + auto [o1, g_id_to_t_id1] = index_fn(); + auto [shard_index1, muted_index1] = std::move(o1); auto rhc = lbt::random_hill_climbing_strategy( - shard_index.shards(), g_id_to_t_id, lbt::muted_index{{}, {}}); + leader_balancer_test_utils::copy_cluster_index(shard_index1.shards()), + std::move(g_id_to_t_id1), + std::move(muted_index1)); cluster::leader_balancer_types::muted_groups_t muted_groups{}; @@ -492,6 +502,7 @@ BOOST_AUTO_TEST_CASE(topic_skew_error) { rhc.apply_movement(*movement_opt); even_shard_con.update_index(*movement_opt); even_topic_con.update_index(*movement_opt); + shard_index.update_index(*movement_opt); muted_groups.add(static_cast(movement_opt->group)); auto new_error = rhc.error(); diff --git a/src/v/cluster/tests/leader_balancer_test.cc b/src/v/cluster/tests/leader_balancer_test.cc index d1529bed11d97..444b1c839cf1e 100644 --- a/src/v/cluster/tests/leader_balancer_test.cc +++ b/src/v/cluster/tests/leader_balancer_test.cc @@ -52,18 +52,22 @@ BOOST_AUTO_TEST_CASE(greedy_movement) { // 2 cores x node // 10 partitions per shard // r=3 (3 replicas) + auto index = leader_balancer_test_utils::make_cluster_index(10, 2, 10, 3); - auto greed = cluster::greedy_balanced_shards(index, {}); + auto greed = cluster::greedy_balanced_shards( + leader_balancer_test_utils::copy_cluster_index(index), {}); BOOST_REQUIRE_EQUAL(greed.error(), 0); // new groups on shard {2, 0} auto shard20 = model::broker_shard{model::node_id{2}, 0}; + index[shard20][raft::group_id(20)] = index[shard20][raft::group_id(3)]; index[shard20][raft::group_id(21)] = index[shard20][raft::group_id(3)]; index[shard20][raft::group_id(22)] = index[shard20][raft::group_id(3)]; - greed = cluster::greedy_balanced_shards(index, {}); + greed = cluster::greedy_balanced_shards( + leader_balancer_test_utils::copy_cluster_index(index), {}); BOOST_REQUIRE_GT(greed.error(), 0); // movement should be _from_ the overloaded shard @@ -146,7 +150,9 @@ static auto from_spec( std::inserter(muted_bs, muted_bs.begin()), [](auto id) { return model::node_id{id}; }); - return std::make_tuple(index, gbs{index, muted_bs}); + auto index_cp = leader_balancer_test_utils::copy_cluster_index(index); + return std::make_tuple( + std::move(index_cp), gbs{std::move(index), muted_bs}); }; /** diff --git a/src/v/cluster/tests/leader_balancer_test_utils.h b/src/v/cluster/tests/leader_balancer_test_utils.h index 4a908563a4e2c..0bff2f84b13eb 100644 --- a/src/v/cluster/tests/leader_balancer_test_utils.h +++ b/src/v/cluster/tests/leader_balancer_test_utils.h @@ -51,4 +51,17 @@ static cluster::leader_balancer_strategy::index_type make_cluster_index( return index; } +static inline cluster::leader_balancer_strategy::index_type copy_cluster_index( + const cluster::leader_balancer_strategy::index_type& c_index) { + cluster::leader_balancer_strategy::index_type index; + + for (const auto& [bs, leaders] : c_index) { + for (const auto& [group_id, replicas] : leaders) { + index[bs][group_id] = replicas; + } + } + + return index; +} + } // namespace leader_balancer_test_utils From 1ee81f170815d34adf737e0874df94914c1b1f6b Mon Sep 17 00:00:00 2001 From: Brandon Allard Date: Fri, 31 May 2024 13:40:53 -0400 Subject: [PATCH 3/3] cluster/tests: remove even_topic_c_bench from leader_balancer_bench The `recommended_assignment` method this benchmarked has since been removed. Hence there is nothing to benchmark here. --- src/v/cluster/tests/leader_balancer_bench.cc | 35 -------------------- 1 file changed, 35 deletions(-) diff --git a/src/v/cluster/tests/leader_balancer_bench.cc b/src/v/cluster/tests/leader_balancer_bench.cc index 5b0210cf93b26..68575d922d418 100644 --- a/src/v/cluster/tests/leader_balancer_bench.cc +++ b/src/v/cluster/tests/leader_balancer_bench.cc @@ -86,38 +86,6 @@ void random_search_eval_bench(bool measure_all) { perf_tests::stop_measuring_time(); } -/* - * Measures the time the even_topic_distributon_constraint takes to generate - * a recommend reassignment in the worst case where it has to iterate through - * every possible reassignment. - */ -void even_topic_c_bench(bool measure_all) { - auto index = leader_balancer_test_utils::make_cluster_index( - node_count, shards_per_node, groups_per_shard, replicas); - - cluster::leader_balancer_types::muted_index mi{{}, {}}; - cluster::leader_balancer_types::shard_index si(std::move(index)); - auto gid_topic = make_gid_to_topic_index(si.shards()); - - if (measure_all) { - perf_tests::start_measuring_time(); - } - - cluster::leader_balancer_types::even_topic_distributon_constraint tdc( - std::move(gid_topic), si, mi); - - if (!measure_all) { - perf_tests::start_measuring_time(); - } - - auto movement = tdc.recommended_reassignment(); - vassert(!movement, "movemement"); - - perf_tests::do_not_optimize(movement); - - perf_tests::stop_measuring_time(); -} - /* * Measures the time needed to randomly generate every possible reassignment for * a cluster. @@ -176,9 +144,6 @@ void balancer_bench(bool measure_all) { PERF_TEST(lb, even_shard_load_movement) { balancer_bench(false); } PERF_TEST(lb, even_shard_load_all) { balancer_bench(true); } -PERF_TEST(lb, even_topic_distribution_movement) { even_topic_c_bench(false); } -PERF_TEST(lb, even_topic_distribution_all) { even_topic_c_bench(true); } - PERF_TEST(lb, random_eval_movement) { random_search_eval_bench< cluster::leader_balancer_types::random_reassignments>(false);