From 37ab8e36e5d70f2f284bf985dc796c5d9c25d393 Mon Sep 17 00:00:00 2001 From: Michal Maslanka Date: Tue, 23 Jan 2024 16:08:57 +0100 Subject: [PATCH] c/health_monitor: changed the partition_status chunk size to 8 With large number of topics that contain a small amount of partitions we may end up wasting a lot of memory using the default `ss::chunked_fifo` chunk size of 128. Change the chunk size of `partition_status` `chunked_fifo` collection to 8 to minimize a waste with large number of small topics. Signed-off-by: Michal Maslanka (cherry picked from commit 75c954933adb9b829add7410645b0f4f97ada79a) --- src/v/cluster/health_monitor_backend.cc | 4 ++-- src/v/cluster/health_monitor_types.cc | 5 +++-- src/v/cluster/health_monitor_types.h | 9 +++++++-- src/v/cluster/tests/health_monitor_test.cc | 2 +- src/v/cluster/tests/partition_balancer_simulator_test.cc | 2 +- src/v/cluster/tests/randoms.h | 9 ++++++--- src/v/cluster/tests/serialization_rt_test.cc | 2 +- src/v/compat/cluster_json.h | 2 +- 8 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/v/cluster/health_monitor_backend.cc b/src/v/cluster/health_monitor_backend.cc index 04073f3f2679..bf8af859d28f 100644 --- a/src/v/cluster/health_monitor_backend.cc +++ b/src/v/cluster/health_monitor_backend.cc @@ -735,8 +735,8 @@ ss::chunked_fifo collect_shard_local_reports( return reports; } -using reports_acc_t = absl:: - node_hash_map>; +using reports_acc_t + = absl::node_hash_map; reports_acc_t reduce_reports_map( reports_acc_t acc, ss::chunked_fifo current_reports) { diff --git a/src/v/cluster/health_monitor_types.cc b/src/v/cluster/health_monitor_types.cc index 67cd66c7e25d..4f3f69d8a3ae 100644 --- a/src/v/cluster/health_monitor_types.cc +++ b/src/v/cluster/health_monitor_types.cc @@ -14,6 +14,7 @@ #include "cluster/errc.h" #include "cluster/node/types.h" #include "features/feature_table.h" +#include "health_monitor_types.h" #include "model/adl_serde.h" #include "model/metadata.h" #include "utils/to_string.h" @@ -156,7 +157,7 @@ topic_status& topic_status::operator=(const topic_status& rhs) { return *this; } - ss::chunked_fifo p; + partition_statuses_t p; p.reserve(rhs.partitions.size()); std::copy( rhs.partitions.begin(), rhs.partitions.end(), std::back_inserter(p)); @@ -167,7 +168,7 @@ topic_status& topic_status::operator=(const topic_status& rhs) { } topic_status::topic_status( - model::topic_namespace tp_ns, ss::chunked_fifo partitions) + model::topic_namespace tp_ns, partition_statuses_t partitions) : tp_ns(std::move(tp_ns)) , partitions(std::move(partitions)) {} diff --git a/src/v/cluster/health_monitor_types.h b/src/v/cluster/health_monitor_types.h index 6a4bd8b613aa..ab57a603116e 100644 --- a/src/v/cluster/health_monitor_types.h +++ b/src/v/cluster/health_monitor_types.h @@ -97,13 +97,18 @@ struct partition_status friend bool operator==(const partition_status&, const partition_status&) = default; }; +/** + * We keep the chunk size in partition status small to make sure that we do not + * waste to much of memory for topics with small number of partitions. + */ +using partition_statuses_t = ss::chunked_fifo; struct topic_status : serde::envelope, serde::compat_version<0>> { static constexpr int8_t current_version = 0; topic_status() = default; - topic_status(model::topic_namespace, ss::chunked_fifo); + topic_status(model::topic_namespace, partition_statuses_t); topic_status& operator=(const topic_status&); topic_status(const topic_status&); topic_status& operator=(topic_status&&) = default; @@ -111,7 +116,7 @@ struct topic_status ~topic_status() = default; model::topic_namespace tp_ns; - ss::chunked_fifo partitions; + partition_statuses_t partitions; friend std::ostream& operator<<(std::ostream&, const topic_status&); friend bool operator==(const topic_status&, const topic_status&); diff --git a/src/v/cluster/tests/health_monitor_test.cc b/src/v/cluster/tests/health_monitor_test.cc index 6c5af1322790..1ab9ea2e8edc 100644 --- a/src/v/cluster/tests/health_monitor_test.cc +++ b/src/v/cluster/tests/health_monitor_test.cc @@ -378,7 +378,7 @@ enum part_status { HEALTHY, LEADERLESS, URP }; topic_status make_ts(ss::sstring name, const std::vector& status_list) { - ss::chunked_fifo statuses; + cluster::partition_statuses_t statuses; partition_id pid{0}; for (auto status : status_list) { diff --git a/src/v/cluster/tests/partition_balancer_simulator_test.cc b/src/v/cluster/tests/partition_balancer_simulator_test.cc index 3df26b6d71de..2711d6c6f730 100644 --- a/src/v/cluster/tests/partition_balancer_simulator_test.cc +++ b/src/v/cluster/tests/partition_balancer_simulator_test.cc @@ -609,7 +609,7 @@ class partition_balancer_sim_fixture { absl::flat_hash_map< model::topic_namespace, - ss::chunked_fifo> + cluster::partition_statuses_t> topic2partitions; for (const auto& [ntp, repl] : replicas) { topic2partitions[model::topic_namespace(ntp.ns, ntp.tp.topic)] diff --git a/src/v/cluster/tests/randoms.h b/src/v/cluster/tests/randoms.h index 935f7888be43..effabd0c3c6b 100644 --- a/src/v/cluster/tests/randoms.h +++ b/src/v/cluster/tests/randoms.h @@ -18,6 +18,8 @@ #include "storage/tests/randoms.h" #include "test_utils/randoms.h" +#include + namespace cluster::node { inline local_state random_local_state() { @@ -60,9 +62,10 @@ inline partition_status random_partition_status() { } inline topic_status random_topic_status() { - return topic_status( - model::random_topic_namespace(), - tests::random_chunked_fifo(random_partition_status)); + auto d = tests::random_vector(random_partition_status); + cluster::partition_statuses_t partitions; + std::move(d.begin(), d.end(), std::back_inserter(partitions)); + return {model::random_topic_namespace(), std::move(partitions)}; } inline drain_manager::drain_status random_drain_status() { diff --git a/src/v/cluster/tests/serialization_rt_test.cc b/src/v/cluster/tests/serialization_rt_test.cc index 360f2103bb52..a0f3be4c89d5 100644 --- a/src/v/cluster/tests/serialization_rt_test.cc +++ b/src/v/cluster/tests/serialization_rt_test.cc @@ -731,7 +731,7 @@ cluster::drain_manager::drain_status random_drain_status() { } cluster::topic_status random_topic_status() { - ss::chunked_fifo partitions; + cluster::partition_statuses_t partitions; for (int i = 0, mi = random_generators::get_int(10); i < mi; i++) { partitions.push_back(cluster::partition_status{ .id = tests::random_named_int(), diff --git a/src/v/compat/cluster_json.h b/src/v/compat/cluster_json.h index a4d4bf1ae814..43ce6acda340 100644 --- a/src/v/compat/cluster_json.h +++ b/src/v/compat/cluster_json.h @@ -398,7 +398,7 @@ inline void read_value(json::Value const& rd, cluster::partition_status& obj) { inline void read_value(json::Value const& rd, cluster::topic_status& obj) { model::topic_namespace tp_ns; - ss::chunked_fifo partitions; + cluster::partition_statuses_t partitions; read_member(rd, "tp_ns", tp_ns); read_member(rd, "partitions", partitions);