From 8757b3ece78e07aa5db1477d895ddb7c4ea44017 Mon Sep 17 00:00:00 2001 From: Bharath Vissapragada Date: Mon, 22 Jan 2024 19:16:29 -0800 Subject: [PATCH 1/5] admin api: skip partition info in /brokers end point fetching partition information via health report API is heavy and brokers API doesn't really use it. This patch avoids fetching that information. (cherry picked from commit 0ecb2a453f30d2d91ac0f957692a49518533e81a) --- src/v/redpanda/admin_server.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/v/redpanda/admin_server.cc b/src/v/redpanda/admin_server.cc index 9d227d69abb86..ba6f86119b2b1 100644 --- a/src/v/redpanda/admin_server.cc +++ b/src/v/redpanda/admin_server.cc @@ -849,6 +849,7 @@ fill_maintenance_status(const cluster::broker_state& b_state) { ss::future> get_brokers(cluster::controller* const controller) { cluster::node_report_filter filter; + filter.include_partitions = cluster::include_partitions_info::no; return controller->get_health_monitor() .local() From bb83fa4fa9def749b450b76f7377a65bbeb8d2da Mon Sep 17 00:00:00 2001 From: Michal Maslanka Date: Tue, 23 Jan 2024 16:08:22 +0100 Subject: [PATCH 2/5] to_string: updated formatter to be chunk size agnostic Signed-off-by: Michal Maslanka (cherry picked from commit 9b5cc59d37392b2e544a1c3da396a3613f8ee002) --- src/v/utils/to_string.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/v/utils/to_string.h b/src/v/utils/to_string.h index 4909a0a560333..9d2f69964b95c 100644 --- a/src/v/utils/to_string.h +++ b/src/v/utils/to_string.h @@ -52,9 +52,9 @@ operator<<(std::ostream& o, const ss::lowres_clock::duration& d) { } // namespace std -template -struct fmt::formatter> { - using type = ss::chunked_fifo; +template +struct fmt::formatter> { + using type = ss::chunked_fifo; constexpr auto parse(format_parse_context& ctx) { return ctx.begin(); } From 168cf6b17a3b6f280a4ff01948211c301531cbd1 Mon Sep 17 00:00:00 2001 From: Michal Maslanka Date: Wed, 24 Jan 2024 09:16:09 +0100 Subject: [PATCH 3/5] json: made chunked_fifo json helpers chunk size agnostic Signed-off-by: Michal Maslanka (cherry picked from commit 22d34a6ebbda65616467549f6555f79fb3674721) --- src/v/compat/json.h | 4 ++-- src/v/json/json.h | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/v/compat/json.h b/src/v/compat/json.h index b0608bce0950d..bad6ed4c3bba4 100644 --- a/src/v/compat/json.h +++ b/src/v/compat/json.h @@ -109,8 +109,8 @@ void read_value(json::Value const& v, std::vector& target) { } } -template -void read_value(json::Value const& v, ss::chunked_fifo& target) { +template +void read_value(json::Value const& v, ss::chunked_fifo& target) { for (auto const& e : v.GetArray()) { auto t = T{}; read_value(e, t); diff --git a/src/v/json/json.h b/src/v/json/json.h index b8f399b8ea9ff..298e96943e2c2 100644 --- a/src/v/json/json.h +++ b/src/v/json/json.h @@ -107,9 +107,10 @@ void rjson_serialize( w.EndArray(); } -template +template void rjson_serialize( - json::Writer& w, const ss::chunked_fifo& v) { + json::Writer& w, + const ss::chunked_fifo& v) { w.StartArray(); for (const auto& e : v) { rjson_serialize(w, e); From 0ccf8fa47d8f08a9f46b1b498b57defa869709c7 Mon Sep 17 00:00:00 2001 From: Michal Maslanka Date: Thu, 25 Jan 2024 08:27:07 +0100 Subject: [PATCH 4/5] serde: made serde write chunk size agnostic Signed-off-by: Michal Maslanka --- src/v/serde/serde.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/v/serde/serde.h b/src/v/serde/serde.h index e1e10df6321a3..128b862121716 100644 --- a/src/v/serde/serde.h +++ b/src/v/serde/serde.h @@ -434,8 +434,8 @@ void write(iobuf& out, fragmented_vector t) { } } -template -void write(iobuf& out, ss::chunked_fifo t) { +template +void write(iobuf& out, ss::chunked_fifo t) { if (unlikely(t.size() > std::numeric_limits::max())) { throw serde_exception(fmt_with_ctx( ssx::sformat, From 37ab8e36e5d70f2f284bf985dc796c5d9c25d393 Mon Sep 17 00:00:00 2001 From: Michal Maslanka Date: Tue, 23 Jan 2024 16:08:57 +0100 Subject: [PATCH 5/5] 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 04073f3f2679e..bf8af859d28f8 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 67cd66c7e25d2..4f3f69d8a3ae9 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 6a4bd8b613aae..ab57a603116e7 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 6c5af1322790e..1ab9ea2e8edcf 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 3df26b6d71de9..2711d6c6f7301 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 935f7888be437..effabd0c3c6b0 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 360f2103bb52b..a0f3be4c89d5e 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 a4d4bf1ae814f..43ce6acda3405 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);