From 8d704f25322927b68f844a91a861dd793b61169c Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 15:25:02 +0300 Subject: [PATCH 01/13] sstable_test_env: Coroutinize and move to .cc test_env::stop() It's going to get larger, so better to move. Also when coroutinized it's goind to be easier to extend. Signed-off-by: Pavel Emelyanov --- test/lib/sstable_test_env.hh | 6 +----- test/lib/test_services.cc | 5 +++++ 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index 923144ac76..553ac1c187 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -79,11 +79,7 @@ public: explicit test_env(test_env_config cfg = {}, sstables::storage_manager* sstm = nullptr) : _impl(std::make_unique(std::move(cfg), sstm)) { } - future<> stop() { - return _impl->mgr.close().finally([this] { - return _impl->semaphore.stop(); - }); - } + future<> stop(); sstables::generation_type new_generation() noexcept { return _impl->new_generation(); diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 9a06cfb91e..9366089876 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -215,6 +215,11 @@ test_env::impl::impl(test_env_config cfg, sstables::storage_manager* sstm) } } +future<> test_env::stop() { + co_await _impl->mgr.close(); + co_await _impl->semaphore.stop(); +} + future<> test_env::do_with_async(noncopyable_function func, test_env_config cfg) { if (!cfg.storage.is_local_type()) { struct test_env_with_cql { From cba8f633f182c7719f95f2e6173e47adb45269ba Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 19:25:21 +0300 Subject: [PATCH 02/13] tests: Split the compaction backlog test case To improve parallelizm of embedded test sub-cases. By coinsidence, indentation fix is not required. Signed-off-by: Pavel Emelyanov --- test/boost/sstable_compaction_test.cc | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test/boost/sstable_compaction_test.cc b/test/boost/sstable_compaction_test.cc index b7e33e5f23..a27d6707d7 100644 --- a/test/boost/sstable_compaction_test.cc +++ b/test/boost/sstable_compaction_test.cc @@ -4453,8 +4453,8 @@ SEASTAR_TEST_CASE(test_major_does_not_miss_data_in_memtable) { }); } -SEASTAR_TEST_CASE(simple_backlog_controller_test) { - auto run_controller_test = [] (sstables::compaction_strategy_type compaction_strategy_type, test_env& env) { +future<> run_controller_test(sstables::compaction_strategy_type compaction_strategy_type) { + return test_env::do_with_async([compaction_strategy_type] (test_env& env) { ///////////// // settings static constexpr float disk_memory_ratio = 78.125; /* AWS I3en is ~78.125 */ @@ -4564,15 +4564,21 @@ SEASTAR_TEST_CASE(simple_backlog_controller_test) { auto max_expected = compaction_strategy_type == sstables::compaction_strategy_type::leveled ? 0.4f : 0.0f; BOOST_REQUIRE(r.normalized_backlog <= max_expected); } - }; - - return test_env::do_with_async([run_controller_test] (test_env& env) { - run_controller_test(sstables::compaction_strategy_type::size_tiered, env); - run_controller_test(sstables::compaction_strategy_type::time_window, env); - run_controller_test(sstables::compaction_strategy_type::leveled, env); }); } +SEASTAR_TEST_CASE(simple_backlog_controller_test_size_tiered) { + return run_controller_test(sstables::compaction_strategy_type::size_tiered); +} + +SEASTAR_TEST_CASE(simple_backlog_controller_test_time_window) { + return run_controller_test(sstables::compaction_strategy_type::time_window); +} + +SEASTAR_TEST_CASE(simple_backlog_controller_test_leveled) { + return run_controller_test(sstables::compaction_strategy_type::leveled); +} + SEASTAR_TEST_CASE(test_compaction_strategy_cleanup_method) { return test_env::do_with_async([] (test_env& env) { constexpr size_t all_files = 64; From 89e253c77e4d8c9609a325512b60866491150aac Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 14:22:35 +0300 Subject: [PATCH 03/13] table_for_tests: Remove unused constructor No code constructs it with just sstables manager argument. Signed-off-by: Pavel Emelyanov --- test/lib/test_services.cc | 7 ------- test/lib/test_services.hh | 2 -- 2 files changed, 9 deletions(-) diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 9366089876..0a09c8f6c0 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -37,13 +37,6 @@ schema_ptr table_for_tests::make_default_schema() { .build(); } -table_for_tests::table_for_tests(sstables::sstables_manager& sstables_manager) - : table_for_tests( - sstables_manager, - make_default_schema() - ) -{ } - class table_for_tests::table_state : public compaction::table_state { table_for_tests::data& _data; sstables::sstables_manager& _sstables_manager; diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index 3efeea2824..f6281b7958 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -55,8 +55,6 @@ struct table_for_tests { static schema_ptr make_default_schema(); - explicit table_for_tests(sstables::sstables_manager& sstables_manager); - explicit table_for_tests(sstables::sstables_manager& sstables_manager, schema_ptr s, std::optional datadir = {}, data_dictionary::storage_options storage = {}); schema_ptr schema() { return _data->s; } From 769d9f17eb32cdb202e179409f5a77f459fdf238 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 14:33:08 +0300 Subject: [PATCH 04/13] table_for_tests: Reuse cache tracker from sstables manager When making table object it needs the cache tracker reference. The table_for_tests keeps one on board, but the very same object already sits on the sstables manager which has public getter. This makes the table_for_tests's cache tracker object not needed. Signed-off-by: Pavel Emelyanov --- test/lib/test_services.cc | 2 +- test/lib/test_services.hh | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 0a09c8f6c0..59fb50db82 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -136,7 +136,7 @@ table_for_tests::table_for_tests(sstables::sstables_manager& sstables_manager, s _data->cfg.cf_stats = &_data->cf_stats; _data->cfg.enable_commitlog = false; _data->cm.enable(); - _data->cf = make_lw_shared(_data->s, _data->cfg, make_lw_shared(), _data->cm, sstables_manager, _data->cl_stats, _data->tracker, nullptr); + _data->cf = make_lw_shared(_data->s, _data->cfg, make_lw_shared(), _data->cm, sstables_manager, _data->cl_stats, sstables_manager.get_cache_tracker(), nullptr); _data->cf->mark_ready_for_writes(nullptr); _data->table_s = std::make_unique(*_data, sstables_manager); _data->cm.add(*_data->table_s); diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index f6281b7958..00aacb0c63 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -39,7 +39,6 @@ struct table_for_tests { struct data { schema_ptr s; reader_concurrency_semaphore semaphore; - cache_tracker tracker; replica::cf_stats cf_stats{0}; replica::column_family::config cfg; cell_locker_stats cl_stats; From 35f7ada94910ef69be96e4375ba084d661b21fd6 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 14:38:51 +0300 Subject: [PATCH 05/13] table_for_tests: Get table directory from table itself Making sstable for a table needs passing table directory as an argument. Current table_for_tests's helper gets the directory from table config, but the very same path sits on the table itself. This makes testing code to construct sstable look closer to the core code and is also the prerequisite for removing the table config from table_for_tests in the future. Signed-off-by: Pavel Emelyanov --- test/lib/test_services.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index 00aacb0c63..9301d6336f 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -74,13 +74,13 @@ struct table_for_tests { sstables::shared_sstable make_sstable() { auto& table = *_data->cf; auto& sstables_manager = table.get_sstables_manager(); - return sstables_manager.make_sstable(_data->s, _data->cfg.datadir, _data->storage, table.calculate_generation_for_new_table()); + return sstables_manager.make_sstable(_data->s, table.dir(), _data->storage, table.calculate_generation_for_new_table()); } sstables::shared_sstable make_sstable(sstables::sstable_version_types version) { auto& table = *_data->cf; auto& sstables_manager = table.get_sstables_manager(); - return sstables_manager.make_sstable(_data->s, _data->cfg.datadir, _data->storage, table.calculate_generation_for_new_table(), sstables::sstable_state::normal, version); + return sstables_manager.make_sstable(_data->s, table.dir(), _data->storage, table.calculate_generation_for_new_table(), sstables::sstable_state::normal, version); } std::function make_sst_factory() { From 76e57cc8053257457874efdb1cb193e0c1b468da Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 14:55:02 +0300 Subject: [PATCH 06/13] table_for_tests: Get concurrency semaphore from table Making compaction permit needs a semaphore. Current code gets it from the table_for_tests, but the very same semaphore reference sits on the table. So get it from table, as the core code does. This will allow removing the dedicated semaphore from table_for_tests in the future. Signed-off-by: Pavel Emelyanov --- test/lib/test_services.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 59fb50db82..c766130711 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -88,7 +88,7 @@ public: return _compaction_strategy_state; } reader_permit make_compaction_reader_permit() const override { - return _data.semaphore.make_tracking_only_permit(&*schema(), "table_for_tests::table_state", db::no_timeout, {}); + return table().compaction_concurrency_semaphore().make_tracking_only_permit(&*schema(), "table_for_tests::table_state", db::no_timeout, {}); } sstables::sstables_manager& get_sstables_manager() noexcept override { return _sstables_manager; From 5ab1af38047fe8a842c7bb29913a814028a998ef Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 14:41:11 +0300 Subject: [PATCH 07/13] table_for_tests: Create table config locally The table_for_tests keeps a copy of table::config on board. That's not "idiomatic" as table config is a temporary object that should only be needed while creating table object. Fortunately, the copy of config on table_for_tests is no longer needed and it can be made temporary. Signed-off-by: Pavel Emelyanov --- test/lib/test_services.cc | 13 +++++++------ test/lib/test_services.hh | 1 - 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index c766130711..8597d57915 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -130,13 +130,14 @@ table_for_tests::table_for_tests(sstables::sstables_manager& sstables_manager, s : _data(make_lw_shared()) { _data->s = s ? s : make_default_schema(); - _data->cfg = replica::table::config{.compaction_concurrency_semaphore = &_data->semaphore}; - _data->cfg.enable_disk_writes = bool(datadir); - _data->cfg.datadir = datadir.value_or(sstring()); - _data->cfg.cf_stats = &_data->cf_stats; - _data->cfg.enable_commitlog = false; + replica::table::config cfg; + cfg = replica::table::config{.compaction_concurrency_semaphore = &_data->semaphore}; + cfg.enable_disk_writes = bool(datadir); + cfg.datadir = datadir.value_or(sstring()); + cfg.cf_stats = &_data->cf_stats; + cfg.enable_commitlog = false; _data->cm.enable(); - _data->cf = make_lw_shared(_data->s, _data->cfg, make_lw_shared(), _data->cm, sstables_manager, _data->cl_stats, sstables_manager.get_cache_tracker(), nullptr); + _data->cf = make_lw_shared(_data->s, std::move(cfg), make_lw_shared(), _data->cm, sstables_manager, _data->cl_stats, sstables_manager.get_cache_tracker(), nullptr); _data->cf->mark_ready_for_writes(nullptr); _data->table_s = std::make_unique(*_data, sstables_manager); _data->cm.add(*_data->table_s); diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index 9301d6336f..111130ee59 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -40,7 +40,6 @@ struct table_for_tests { schema_ptr s; reader_concurrency_semaphore semaphore; replica::cf_stats cf_stats{0}; - replica::column_family::config cfg; cell_locker_stats cl_stats; tasks::task_manager tm; compaction_manager cm{tm, compaction_manager::for_testing_tag{}}; From 21998296a7f3523fd1df4a00cb41f50619948a93 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 14:51:09 +0300 Subject: [PATCH 08/13] table_for_tests: Require config argument to make table This is the continuation of the previous patch. Make the caller of table_for_tests constructor provide the table::config. This makes the table_for_tests constructor shorter and more self-contained. Also, the caller now needs to provide the reference to reader concurrency semaphore, and that's good news, because the only caller for today is the sstables::test_env that does have it. This makes the semaphore sitting on table_for_tests itself unused and it will be removed eventually. Signed-off-by: Pavel Emelyanov --- test/lib/sstable_test_env.hh | 10 ++++++++-- test/lib/test_services.cc | 9 ++------- test/lib/test_services.hh | 2 +- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index 553ac1c187..a48979c69e 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -209,11 +209,17 @@ public: } table_for_tests make_table_for_tests(schema_ptr s, sstring dir) { - return table_for_tests(manager(), s, std::move(dir), _impl->storage); + auto cfg = make_table_config(); + cfg.datadir = dir; + cfg.enable_commitlog = false; + return table_for_tests(manager(), s, std::move(cfg), _impl->storage); } table_for_tests make_table_for_tests(schema_ptr s = nullptr) { - return table_for_tests(manager(), s, tempdir().path().native(), _impl->storage); + auto cfg = make_table_config(); + cfg.datadir = _impl->dir.path().native(); + cfg.enable_commitlog = false; + return table_for_tests(manager(), s, std::move(cfg), _impl->storage); } }; diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 8597d57915..1c5e920524 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -126,16 +126,11 @@ public: } }; -table_for_tests::table_for_tests(sstables::sstables_manager& sstables_manager, schema_ptr s, std::optional datadir, data_dictionary::storage_options storage) +table_for_tests::table_for_tests(sstables::sstables_manager& sstables_manager, schema_ptr s, replica::table::config cfg, data_dictionary::storage_options storage) : _data(make_lw_shared()) { - _data->s = s ? s : make_default_schema(); - replica::table::config cfg; - cfg = replica::table::config{.compaction_concurrency_semaphore = &_data->semaphore}; - cfg.enable_disk_writes = bool(datadir); - cfg.datadir = datadir.value_or(sstring()); cfg.cf_stats = &_data->cf_stats; - cfg.enable_commitlog = false; + _data->s = s ? s : make_default_schema(); _data->cm.enable(); _data->cf = make_lw_shared(_data->s, std::move(cfg), make_lw_shared(), _data->cm, sstables_manager, _data->cl_stats, sstables_manager.get_cache_tracker(), nullptr); _data->cf->mark_ready_for_writes(nullptr); diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index 111130ee59..f8f595049c 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -53,7 +53,7 @@ struct table_for_tests { static schema_ptr make_default_schema(); - explicit table_for_tests(sstables::sstables_manager& sstables_manager, schema_ptr s, std::optional datadir = {}, data_dictionary::storage_options storage = {}); + explicit table_for_tests(sstables::sstables_manager& sstables_manager, schema_ptr s, replica::table::config cfg, data_dictionary::storage_options storage = {}); schema_ptr schema() { return _data->s; } From ac45aae0c444567d26fc774c60257ee9dbbec919 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 14:55:17 +0300 Subject: [PATCH 09/13] table_for_tests: Ditch on-board concurrency semaphore It's not used any longer and can be removed. This make table_for_tests stopping code a bit shorter as well. Signed-off-by: Pavel Emelyanov --- test/lib/test_services.cc | 3 +-- test/lib/test_services.hh | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 1c5e920524..f24a724f69 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -26,7 +26,6 @@ static const sstring some_keyspace("ks"); static const sstring some_column_family("cf"); table_for_tests::data::data() - : semaphore(reader_concurrency_semaphore::no_limits{}, "table_for_tests") { } table_for_tests::data::~data() {} @@ -146,7 +145,7 @@ compaction::table_state& table_for_tests::as_table_state() noexcept { future<> table_for_tests::stop() { auto data = _data; co_await data->cm.remove(*data->table_s); - co_await when_all_succeed(data->cm.stop(), data->semaphore.stop()).discard_result(); + co_await data->cm.stop(); } void table_for_tests::set_tombstone_gc_enabled(bool tombstone_gc_enabled) noexcept { diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index f8f595049c..ccb476d19c 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -38,7 +38,6 @@ struct table_for_tests { class table_state; struct data { schema_ptr s; - reader_concurrency_semaphore semaphore; replica::cf_stats cf_stats{0}; cell_locker_stats cl_stats; tasks::task_manager tm; From e71409df388735d7b17b4d5b26acdff9cf9acaf9 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 15:04:18 +0300 Subject: [PATCH 10/13] table_for_tests: Get compaction manager from table There's table_for_tests::get_compaction_manager() helper that's excessive as compaction manager reference can be provided by the wrapped table object itself. Signed-off-by: Pavel Emelyanov --- test/boost/sstable_compaction_test.cc | 22 +++++++++++----------- test/lib/test_services.hh | 2 -- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/test/boost/sstable_compaction_test.cc b/test/boost/sstable_compaction_test.cc index a27d6707d7..dd7d21397f 100644 --- a/test/boost/sstable_compaction_test.cc +++ b/test/boost/sstable_compaction_test.cc @@ -114,7 +114,7 @@ static future compact_sstables(sstables::compaction_descriptor descriptor, table_for_tests t, std::function creator, sstables::compaction_sstable_replacer_fn replacer = sstables::replacer_fn_no_op(), can_purge_tombstones can_purge = can_purge_tombstones::yes) { - return compact_sstables(t.get_compaction_manager(), std::move(descriptor), t.as_table_state(), std::move(creator), std::move(replacer), can_purge); + return compact_sstables(t->get_compaction_manager(), std::move(descriptor), t.as_table_state(), std::move(creator), std::move(replacer), can_purge); } class strategy_control_for_test : public strategy_control { @@ -164,7 +164,7 @@ SEASTAR_TEST_CASE(compaction_manager_basic_test) { {{"p1", utf8_type}}, {{"c1", utf8_type}}, {{"r1", int32_type}}, {}, utf8_type); auto cf = env.make_table_for_tests(s); - auto& cm = cf.get_compaction_manager(); + auto& cm = cf->get_compaction_manager(); auto close_cf = deferred_stop(cf); cf->set_compaction_strategy(sstables::compaction_strategy_type::size_tiered); auto sst_gen = cf.make_sst_factory(); @@ -2123,7 +2123,7 @@ SEASTAR_TEST_CASE(sstable_scrub_validate_mode_test) { sstables::compaction_type_options::scrub opts = { .operation_mode = sstables::compaction_type_options::scrub::mode::validate, }; - table.get_compaction_manager().perform_sstable_scrub(ts, opts).get(); + table->get_compaction_manager().perform_sstable_scrub(ts, opts).get(); BOOST_REQUIRE(sst->is_quarantined()); BOOST_REQUIRE(in_strategy_sstables(ts).empty()); @@ -2284,7 +2284,7 @@ SEASTAR_TEST_CASE(sstable_scrub_skip_mode_test) { auto table = env.make_table_for_tests(schema); auto close_cf = deferred_stop(table); table->start(); - auto& compaction_manager = table.get_compaction_manager(); + auto& compaction_manager = table->get_compaction_manager(); table->add_sstable_and_update_cache(sst).get(); @@ -2375,7 +2375,7 @@ SEASTAR_TEST_CASE(sstable_scrub_segregate_mode_test) { auto table = env.make_table_for_tests(schema); auto close_cf = deferred_stop(table); table->start(); - auto& compaction_manager = table.get_compaction_manager(); + auto& compaction_manager = table->get_compaction_manager(); table->add_sstable_and_update_cache(sst).get(); @@ -2481,7 +2481,7 @@ SEASTAR_TEST_CASE(sstable_scrub_quarantine_mode_test) { auto table = env.make_table_for_tests(schema); auto close_cf = deferred_stop(table); table->start(); - auto& compaction_manager = table.get_compaction_manager(); + auto& compaction_manager = table->get_compaction_manager(); table->add_sstable_and_update_cache(sst).get(); @@ -2812,7 +2812,7 @@ SEASTAR_TEST_CASE(sstable_run_based_compaction_test) { cf->start(); cf->set_compaction_strategy(sstables::compaction_strategy_type::size_tiered); auto compact = [&, s] (std::vector all, auto replacer) -> std::vector { - return compact_sstables(cf.get_compaction_manager(), sstables::compaction_descriptor(std::move(all), 1, 0), cf.as_table_state(), sst_gen, replacer).get0().new_sstables; + return compact_sstables(cf->get_compaction_manager(), sstables::compaction_descriptor(std::move(all), 1, 0), cf.as_table_state(), sst_gen, replacer).get0().new_sstables; }; auto make_insert = [&] (const dht::decorated_key& key) { mutation m(s, key); @@ -2835,7 +2835,7 @@ SEASTAR_TEST_CASE(sstable_run_based_compaction_test) { sstables.insert(new_sst); } column_family_test(cf).rebuild_sstable_list(cf.as_table_state(), new_sstables, old_sstables).get(); - compaction_manager_test(cf.get_compaction_manager()).propagate_replacement(cf.as_table_state(), old_sstables, new_sstables); + compaction_manager_test(cf->get_compaction_manager()).propagate_replacement(cf.as_table_state(), old_sstables, new_sstables); }; auto do_incremental_replace = [&] (auto old_sstables, auto new_sstables, auto& expected_sst, auto& closed_sstables_tracker) { @@ -3008,7 +3008,7 @@ SEASTAR_TEST_CASE(backlog_tracker_correctness_after_changing_compaction_strategy BOOST_REQUIRE(ret.new_sstables.size() == 1); } // triggers code that iterates through registered compactions. - cf._data->cm.backlog(); + cf->get_compaction_manager().backlog(); cf.as_table_state().get_backlog_tracker().backlog(); }); } @@ -3044,7 +3044,7 @@ SEASTAR_TEST_CASE(partial_sstable_run_filtered_out_test) { BOOST_REQUIRE(generation_exists(partial_sstable_run_sst->generation())); // register partial sstable run - auto cm_test = compaction_manager_test(cf.get_compaction_manager()); + auto cm_test = compaction_manager_test(cf->get_compaction_manager()); cm_test.run(partial_sstable_run_identifier, cf.as_table_state(), [&cf] (sstables::compaction_data&) { return cf->compact_all_sstables(); }).get(); @@ -3383,7 +3383,7 @@ SEASTAR_TEST_CASE(autocompaction_control_test) { .build(); auto cf = env.make_table_for_tests(s); - auto& cm = cf.get_compaction_manager(); + auto& cm = cf->get_compaction_manager(); auto close_cf = deferred_stop(cf); cf->set_compaction_strategy(sstables::compaction_strategy_type::size_tiered); diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index ccb476d19c..babb3d48a5 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -63,8 +63,6 @@ struct table_for_tests { replica::column_family& operator*() { return *_data->cf; } replica::column_family* operator->() { return _data->cf.get(); } - compaction_manager& get_compaction_manager() noexcept { return _data->cm; } - compaction::table_state& as_table_state() noexcept; future<> stop(); From b96d39e63a486bb2c3b0605c38cfcc7467b013cb Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 20:48:09 +0300 Subject: [PATCH 11/13] table_for_tests: Stop table on stop Next patches will stop using compaction manager from table_for_tests in favor of external one (spoiler: the one from sstables::test_env), thus the compaction manager would outsurvive the table_for_tests object and the table object wrapped by it. So in order for the table_for_tests to stop correctly, it also needs to stop the wrapped table too. Signed-off-by: Pavel Emelyanov --- test/lib/test_services.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index f24a724f69..0b1b1590ff 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -146,6 +146,7 @@ future<> table_for_tests::stop() { auto data = _data; co_await data->cm.remove(*data->table_s); co_await data->cm.stop(); + co_await data->cf->stop(); } void table_for_tests::set_tombstone_gc_enabled(bool tombstone_gc_enabled) noexcept { From 2c78b46c784db7e664feac9c85d4bb18ad60f655 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 16:21:44 +0300 Subject: [PATCH 12/13] sstables::test_env: Carry compaction manager on board Most of the test cases that use sstables::test_env do not mess with table objects, they only need sstables. However, compaction test cases do need table objects and, respectively, a compaction manager instance. Today those test cases create compaction manager instance for each table they create, but that's a bit heaviweight and doesn't work the way core code works. This patch prepares the sstables::test_env to provide compaction manager on demand by starting it as soon as it's asked to create table object. For now this compaction manager is unused, but it will be in next patch. Signed-off-by: Pavel Emelyanov --- test/lib/sstable_test_env.hh | 18 ++++++++++++++++++ test/lib/test_services.cc | 10 ++++++++++ 2 files changed, 28 insertions(+) diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index a48979c69e..9f74cd215f 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -20,6 +20,7 @@ #include "sstables/version.hh" #include "sstables/sstable_directory.hh" #include "replica/database.hh" +#include "compaction/compaction_manager.hh" #include "test/lib/tmpdir.hh" #include "test/lib/test_services.hh" @@ -44,6 +45,18 @@ public: } }; +class test_env_compaction_manager { + tasks::task_manager _tm; + compaction_manager _cm; + +public: + test_env_compaction_manager() + : _cm(_tm, compaction_manager::for_testing_tag{}) + {} + + compaction_manager& get_compaction_manager() { return _cm; } +}; + struct test_env_config { db::large_data_handler* large_data_handler = nullptr; data_dictionary::storage_options storage; // will be local by default @@ -61,6 +74,7 @@ class test_env { gms::feature_service feature_service; db::nop_large_data_handler nop_ld_handler; test_env_sstables_manager mgr; + std::unique_ptr cmgr; reader_concurrency_semaphore semaphore; sstables::sstable_generation_generator gen{0}; sstables::uuid_identifiers use_uuid; @@ -75,6 +89,8 @@ class test_env { } }; std::unique_ptr _impl; + + void maybe_start_compaction_manager(); public: explicit test_env(test_env_config cfg = {}, sstables::storage_manager* sstm = nullptr) : _impl(std::make_unique(std::move(cfg), sstm)) { } @@ -209,6 +225,7 @@ public: } table_for_tests make_table_for_tests(schema_ptr s, sstring dir) { + maybe_start_compaction_manager(); auto cfg = make_table_config(); cfg.datadir = dir; cfg.enable_commitlog = false; @@ -216,6 +233,7 @@ public: } table_for_tests make_table_for_tests(schema_ptr s = nullptr) { + maybe_start_compaction_manager(); auto cfg = make_table_config(); cfg.datadir = _impl->dir.path().native(); cfg.enable_commitlog = false; diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 0b1b1590ff..3eb2fdeda8 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -204,7 +204,17 @@ test_env::impl::impl(test_env_config cfg, sstables::storage_manager* sstm) } } +void test_env::maybe_start_compaction_manager() { + if (!_impl->cmgr) { + _impl->cmgr = std::make_unique(); + _impl->cmgr->get_compaction_manager().enable(); + } +} + future<> test_env::stop() { + if (_impl->cmgr) { + co_await _impl->cmgr->get_compaction_manager().stop(); + } co_await _impl->mgr.close(); co_await _impl->semaphore.stop(); } From 4db80ed61f04775e324589fa91c434981084d0e8 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 25 Oct 2023 16:28:28 +0300 Subject: [PATCH 13/13] table_for_tests: Use test_env's compaction manager Now when the sstables::test_env provides the compaction manager instance, the table_for_tests can start using it and can remove c.m. and the sidecar task_manager. Signed-off-by: Pavel Emelyanov --- test/lib/sstable_test_env.hh | 4 ++-- test/lib/test_services.cc | 10 ++++------ test/lib/test_services.hh | 4 +--- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index 9f74cd215f..d2f425356d 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -229,7 +229,7 @@ public: auto cfg = make_table_config(); cfg.datadir = dir; cfg.enable_commitlog = false; - return table_for_tests(manager(), s, std::move(cfg), _impl->storage); + return table_for_tests(manager(), _impl->cmgr->get_compaction_manager(), s, std::move(cfg), _impl->storage); } table_for_tests make_table_for_tests(schema_ptr s = nullptr) { @@ -237,7 +237,7 @@ public: auto cfg = make_table_config(); cfg.datadir = _impl->dir.path().native(); cfg.enable_commitlog = false; - return table_for_tests(manager(), s, std::move(cfg), _impl->storage); + return table_for_tests(manager(), _impl->cmgr->get_compaction_manager(), s, std::move(cfg), _impl->storage); } }; diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 3eb2fdeda8..2363291cd8 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -125,16 +125,15 @@ public: } }; -table_for_tests::table_for_tests(sstables::sstables_manager& sstables_manager, schema_ptr s, replica::table::config cfg, data_dictionary::storage_options storage) +table_for_tests::table_for_tests(sstables::sstables_manager& sstables_manager, compaction_manager& cm, schema_ptr s, replica::table::config cfg, data_dictionary::storage_options storage) : _data(make_lw_shared()) { cfg.cf_stats = &_data->cf_stats; _data->s = s ? s : make_default_schema(); - _data->cm.enable(); - _data->cf = make_lw_shared(_data->s, std::move(cfg), make_lw_shared(), _data->cm, sstables_manager, _data->cl_stats, sstables_manager.get_cache_tracker(), nullptr); + _data->cf = make_lw_shared(_data->s, std::move(cfg), make_lw_shared(), cm, sstables_manager, _data->cl_stats, sstables_manager.get_cache_tracker(), nullptr); _data->cf->mark_ready_for_writes(nullptr); _data->table_s = std::make_unique(*_data, sstables_manager); - _data->cm.add(*_data->table_s); + cm.add(*_data->table_s); _data->storage = std::move(storage); } @@ -144,8 +143,7 @@ compaction::table_state& table_for_tests::as_table_state() noexcept { future<> table_for_tests::stop() { auto data = _data; - co_await data->cm.remove(*data->table_s); - co_await data->cm.stop(); + co_await data->cf->get_compaction_manager().remove(*data->table_s); co_await data->cf->stop(); } diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index babb3d48a5..d88bbdbbca 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -40,8 +40,6 @@ struct table_for_tests { schema_ptr s; replica::cf_stats cf_stats{0}; cell_locker_stats cl_stats; - tasks::task_manager tm; - compaction_manager cm{tm, compaction_manager::for_testing_tag{}}; lw_shared_ptr cf; std::unique_ptr table_s; data_dictionary::storage_options storage; @@ -52,7 +50,7 @@ struct table_for_tests { static schema_ptr make_default_schema(); - explicit table_for_tests(sstables::sstables_manager& sstables_manager, schema_ptr s, replica::table::config cfg, data_dictionary::storage_options storage = {}); + explicit table_for_tests(sstables::sstables_manager& sstables_manager, compaction_manager& cm, schema_ptr s, replica::table::config cfg, data_dictionary::storage_options storage = {}); schema_ptr schema() { return _data->s; }