From d6fe99abc452331ff3ad44ac8553f7fe076cb11a Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Mon, 13 Feb 2023 14:09:52 -0300 Subject: [PATCH] replica: table: Update stats for newly added SSTables Patch 55a8421e3d35b85ca1497d6 fixed an inefficiency when rebuilding statistics with many compaction groups, but it incorrectly removed the update for newly added SSTables. This patch restores it. When a new SSTable is added to any of the groups, the stats are incrementally updated (as before). On compaction completion, statistics are still rebuilt by simply iterating through each group, which keeps track of its own stats. Unit tests are added to guarantee the stats are correct both after compaction completion and memtable flush. Fixes #12808. Signed-off-by: Raphael S. Carvalho Closes #12834 --- replica/database.hh | 1 + replica/table.cc | 8 ++++++++ test/boost/sstable_compaction_test.cc | 12 +++++++++--- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/replica/database.hh b/replica/database.hh index dbd0f9210e..75fc94ac3c 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -543,6 +543,7 @@ private: bool cache_enabled() const { return _config.enable_cache && _schema->caching_options().enabled(); } + void update_stats_for_new_sstable(const sstables::shared_sstable& sst) 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 05e3792017..2763ad22b0 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -546,6 +546,12 @@ future<> table::parallel_foreach_compaction_group(std::function(compact }); } +void table::update_stats_for_new_sstable(const sstables::shared_sstable& sst) noexcept { + _stats.live_disk_space_used += sst->bytes_on_disk(); + _stats.total_disk_space_used += sst->bytes_on_disk(); + _stats.live_sstable_count++; +} + future<> table::do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::offstrategy offstrategy) { auto permit = co_await seastar::get_units(_sstable_set_mutation_sem, 1); @@ -558,6 +564,7 @@ table::do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::o } else { add_maintenance_sstable(cg, sst); } + update_stats_for_new_sstable(sst); }), dht::partition_range::make({sst->get_first_decorated_key(), true}, {sst->get_last_decorated_key(), true})); } @@ -597,6 +604,7 @@ table::update_cache(compaction_group& cg, lw_shared_ptr m, std::vector auto adder = row_cache::external_updater([this, m, ssts = std::move(ssts), new_ssts_ms = std::move(*ms_opt), &cg] () mutable { for (auto& sst : ssts) { add_sstable(cg, sst); + update_stats_for_new_sstable(sst); } m->mark_flushed(std::move(new_ssts_ms)); try_trigger_compaction(cg); diff --git a/test/boost/sstable_compaction_test.cc b/test/boost/sstable_compaction_test.cc index 4431b4c3fe..162eb845e3 100644 --- a/test/boost/sstable_compaction_test.cc +++ b/test/boost/sstable_compaction_test.cc @@ -142,6 +142,12 @@ static std::unique_ptr make_strategy_control_for_test(bool has return std::make_unique(has_ongoing_compaction); } +static void assert_table_sstable_count(table_for_tests& t, size_t expected_count) { + testlog.info("sstable_set_size={}, live_sstable_count={}, expected={}", t->get_sstables()->size(), t->get_stats().live_sstable_count, expected_count); + BOOST_REQUIRE(t->get_sstables()->size() == expected_count); + BOOST_REQUIRE(t->get_stats().live_sstable_count == expected_count); +} + SEASTAR_TEST_CASE(compaction_manager_basic_test) { return test_env::do_with_async([] (test_env& env) { BOOST_REQUIRE(smp::count == 1); @@ -3896,7 +3902,7 @@ SEASTAR_TEST_CASE(test_twcs_interposer_on_memtable_flush) { auto expected_ssts = (split_during_flush) ? target_windows_span : 1; testlog.info("split_during_flush={}, actual={}, expected={}", split_during_flush, cf->get_sstables()->size(), expected_ssts); - BOOST_REQUIRE(cf->get_sstables()->size() == expected_ssts); + assert_table_sstable_count(cf, expected_ssts); }; test_interposer_on_flush(true); @@ -4623,7 +4629,7 @@ SEASTAR_TEST_CASE(test_major_does_not_miss_data_in_memtable) { }(); auto sst = make_sstable_containing(sst_gen, {std::move(row_mut)}); cf->add_sstable_and_update_cache(sst).get(); - BOOST_REQUIRE(cf->get_sstables()->size() == 1); + assert_table_sstable_count(cf, 1); auto deletion_mut = [&] () { mutation m(s, pkey); @@ -4634,7 +4640,7 @@ SEASTAR_TEST_CASE(test_major_does_not_miss_data_in_memtable) { cf->apply(deletion_mut); cf->compact_all_sstables().get(); - BOOST_REQUIRE(cf->get_sstables()->size() == 1); + assert_table_sstable_count(cf, 1); auto new_sst = *(cf->get_sstables()->begin()); BOOST_REQUIRE(new_sst->generation() != sst->generation()); assert_that(sstable_reader(new_sst, s, env.make_reader_permit()))