From 529a1239a98eefd2b1aa6a6785e8c4b88d43de60 Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Thu, 2 Feb 2023 16:38:45 -0300 Subject: [PATCH 1/2] table: Fix disk-space related metrics total disk space used metric is incorrectly telling the amount of disk space ever used, which is wrong. It should tell the size of all sstables being used + the ones waiting to be deleted. live disk space used, by this defition, shouldn't account the ones waiting to be deleted. and live sstable count, shouldn't account sstables waiting to be deleted. Fix all that. Fixes #12717. Signed-off-by: Raphael S. Carvalho --- replica/table.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/replica/table.cc b/replica/table.cc index 7b9d257810..d1fa4fc9ae 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -988,17 +988,16 @@ void table::set_metrics() { } void table::rebuild_statistics() { - // zeroing live_disk_space_used and live_sstable_count because the - // sstable list was re-created _stats.live_disk_space_used = 0; _stats.live_sstable_count = 0; + _stats.total_disk_space_used = 0; _sstables->for_each_sstable([this] (const sstables::shared_sstable& tab) { update_stats_for_new_sstable(tab->bytes_on_disk()); }); for (const compaction_group_ptr& cg : compaction_groups()) { for (auto& tab: cg->compacted_undeleted_sstables()) { - update_stats_for_new_sstable(tab->bytes_on_disk()); + _stats.total_disk_space_used += tab->bytes_on_disk(); } } } From 55a8421e3d35b85ca1497d615540aafe2e02bffd Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Thu, 2 Feb 2023 17:10:11 -0300 Subject: [PATCH 2/2] table: Fix inefficiency when rebuilding statistics with compaction groups Whenever any compaction group has its SSTable set updated, table's rebuild_statistics() is called and it inefficiently iterates through SSTable set of all compaction groups. Now each compaction group keeps track of its statistics, such that table's rebuild_statistics() only need to sum them up. Signed-off-by: Raphael S. Carvalho --- replica/compaction_group.hh | 7 ++++++ replica/database.hh | 1 - replica/table.cc | 46 +++++++++++++++++++++++++------------ 3 files changed, 38 insertions(+), 16 deletions(-) diff --git a/replica/compaction_group.hh b/replica/compaction_group.hh index 51c3f225a8..beb4291b91 100644 --- a/replica/compaction_group.hh +++ b/replica/compaction_group.hh @@ -45,6 +45,8 @@ class compaction_group { // have not been deleted yet, so must not GC any tombstones in other sstables // that may delete data in these sstables: std::vector _sstables_compacted_but_not_deleted; + uint64_t _main_set_disk_space_used = 0; + uint64_t _maintenance_set_disk_space_used = 0; private: // Adds new sstable to the set of sstables // Doesn't update the cache. The cache must be synchronized in order for reads to see @@ -57,6 +59,7 @@ private: enable_backlog_tracker backlog_tracker); // Update compaction backlog tracker with the same changes applied to the underlying sstable set. void backlog_tracker_adjust_charges(const std::vector& old_sstables, const std::vector& new_sstables); + static uint64_t calculate_disk_space_used_for(const sstables::sstable_set& set); public: compaction_group(table& t, dht::token_range token_range); @@ -110,6 +113,10 @@ public: compaction_backlog_tracker& get_backlog_tracker(); + size_t live_sstable_count() const noexcept; + uint64_t live_disk_space_used() const noexcept; + uint64_t total_disk_space_used() const noexcept; + compaction::table_state& as_table_state() const noexcept; }; diff --git a/replica/database.hh b/replica/database.hh index c28b3023c9..dbd0f9210e 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -543,7 +543,6 @@ private: bool cache_enabled() const { return _config.enable_cache && _schema->caching_options().enabled(); } - void update_stats_for_new_sstable(uint64_t disk_space_used_by_sstable) noexcept; future<> do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::offstrategy offstrategy); // Helpers which add sstable on behalf of a compaction group and refreshes compound set. void add_sstable(compaction_group& cg, sstables::shared_sstable sstable); diff --git a/replica/table.cc b/replica/table.cc index d1fa4fc9ae..987e2b29f5 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -407,12 +407,6 @@ void table::notify_bootstrap_or_replace_end() { trigger_offstrategy_compaction(); } -void table::update_stats_for_new_sstable(uint64_t disk_space_used_by_sstable) noexcept { - _stats.live_disk_space_used += disk_space_used_by_sstable; - _stats.total_disk_space_used += disk_space_used_by_sstable; - _stats.live_sstable_count++; -} - inline void table::add_sstable_to_backlog_tracker(compaction_backlog_tracker& tracker, sstables::shared_sstable sstable) { tracker.replace_sstables({}, {std::move(sstable)}); } @@ -438,14 +432,13 @@ compaction_group::do_add_sstable(lw_shared_ptr sstables, if (backlog_tracker) { table::add_sstable_to_backlog_tracker(get_backlog_tracker(), sstable); } - // update sstable set last in case either updating - // staging sstables or backlog tracker throws - _t.update_stats_for_new_sstable(sstable->bytes_on_disk()); return new_sstables; } void compaction_group::add_sstable(sstables::shared_sstable sstable) { + auto sstable_size = sstable->bytes_on_disk(); _main_sstables = do_add_sstable(_main_sstables, std::move(sstable), enable_backlog_tracker::yes); + _main_set_disk_space_used += sstable_size; } const lw_shared_ptr& compaction_group::main_sstables() const noexcept { @@ -454,10 +447,13 @@ const lw_shared_ptr& compaction_group::main_sstables() co void compaction_group::set_main_sstables(lw_shared_ptr new_main_sstables) { _main_sstables = std::move(new_main_sstables); + _main_set_disk_space_used = calculate_disk_space_used_for(*_main_sstables); } void compaction_group::add_maintenance_sstable(sstables::shared_sstable sst) { + auto sstable_size = sst->bytes_on_disk(); _maintenance_sstables = do_add_sstable(_maintenance_sstables, std::move(sst), enable_backlog_tracker::no); + _maintenance_set_disk_space_used += sstable_size; } const lw_shared_ptr& compaction_group::maintenance_sstables() const noexcept { @@ -466,6 +462,7 @@ const lw_shared_ptr& compaction_group::maintenance_sstabl void compaction_group::set_maintenance_sstables(lw_shared_ptr new_maintenance_sstables) { _maintenance_sstables = std::move(new_maintenance_sstables); + _maintenance_set_disk_space_used = calculate_disk_space_used_for(*_maintenance_sstables); } void table::add_sstable(compaction_group& cg, sstables::shared_sstable sstable) { @@ -987,18 +984,37 @@ void table::set_metrics() { } } +size_t compaction_group::live_sstable_count() const noexcept { + // FIXME: switch to sstable_set::size() once available. + return _main_sstables->all()->size() + _maintenance_sstables->all()->size(); +} + +uint64_t compaction_group::live_disk_space_used() const noexcept { + return _main_set_disk_space_used + _maintenance_set_disk_space_used; +} + +uint64_t compaction_group::total_disk_space_used() const noexcept { + return live_disk_space_used() + boost::accumulate(_sstables_compacted_but_not_deleted | boost::adaptors::transformed(std::mem_fn(&sstables::sstable::bytes_on_disk)), uint64_t(0)); +} + +uint64_t compaction_group::calculate_disk_space_used_for(const sstables::sstable_set& set) { + uint64_t disk_space_used = 0; + + set.for_each_sstable([&] (const sstables::shared_sstable& sst) { + disk_space_used += sst->bytes_on_disk(); + }); + return disk_space_used; +} + void table::rebuild_statistics() { _stats.live_disk_space_used = 0; _stats.live_sstable_count = 0; _stats.total_disk_space_used = 0; - _sstables->for_each_sstable([this] (const sstables::shared_sstable& tab) { - update_stats_for_new_sstable(tab->bytes_on_disk()); - }); for (const compaction_group_ptr& cg : compaction_groups()) { - for (auto& tab: cg->compacted_undeleted_sstables()) { - _stats.total_disk_space_used += tab->bytes_on_disk(); - } + _stats.live_disk_space_used += cg->live_disk_space_used(); + _stats.total_disk_space_used += cg->total_disk_space_used(); + _stats.live_sstable_count += cg->live_sstable_count(); } }