From 3f354c07a36fa8c92c85142a64bf1bdce5ca8a0d Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Nov 2023 13:48:29 +0300 Subject: [PATCH 1/7] test/sstable_compaction_test: Remove unused tracker allocation The sstable_run_based_compaction_test case allocates the tracker but doesn't use it. Probably was left after the case was patched to use make_table_for_tests() helper. Signed-off-by: Pavel Emelyanov --- test/boost/sstable_compaction_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/boost/sstable_compaction_test.cc b/test/boost/sstable_compaction_test.cc index dd7d21397f..d86f89a4ec 100644 --- a/test/boost/sstable_compaction_test.cc +++ b/test/boost/sstable_compaction_test.cc @@ -2806,7 +2806,6 @@ SEASTAR_TEST_CASE(sstable_run_based_compaction_test) { auto sst_gen = env.make_sst_factory(s); - auto tracker = make_lw_shared(); auto cf = env.make_table_for_tests(s); auto close_cf = deferred_stop(cf); cf->start(); From 19b524d0f309d38bb5c0365bc5e73594dc97f71b Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Nov 2023 12:29:23 +0300 Subject: [PATCH 2/7] sstable_test_env: Tune up maybe_start_compaction_manager() method Make it public and add `bool enable` flag so that test cases could start the compaction manager (to call make_table_for_tests() later) but keep it disabled for their testing purposes. Signed-off-by: Pavel Emelyanov --- test/lib/sstable_test_env.hh | 4 ++-- test/lib/test_services.cc | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index d2f425356d..5b513154a5 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -89,10 +89,10 @@ class test_env { } }; std::unique_ptr _impl; - - void maybe_start_compaction_manager(); public: + void maybe_start_compaction_manager(bool enable = true); + explicit test_env(test_env_config cfg = {}, sstables::storage_manager* sstm = nullptr) : _impl(std::make_unique(std::move(cfg), sstm)) { } future<> stop(); diff --git a/test/lib/test_services.cc b/test/lib/test_services.cc index 2363291cd8..7b38f5e960 100644 --- a/test/lib/test_services.cc +++ b/test/lib/test_services.cc @@ -202,10 +202,12 @@ test_env::impl::impl(test_env_config cfg, sstables::storage_manager* sstm) } } -void test_env::maybe_start_compaction_manager() { +void test_env::maybe_start_compaction_manager(bool enable) { if (!_impl->cmgr) { _impl->cmgr = std::make_unique(); - _impl->cmgr->get_compaction_manager().enable(); + if (enable) { + _impl->cmgr->get_compaction_manager().enable(); + } } } From 3021fb7b6cc41f7e09520e8445fa6b41363c2117 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Nov 2023 13:47:55 +0300 Subject: [PATCH 3/7] sstable_test_env: Add test_env_compaction_manager() getter Signed-off-by: Pavel Emelyanov --- test/lib/sstable_test_env.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index 5b513154a5..9327a6039f 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -179,6 +179,7 @@ public: } test_env_sstables_manager& manager() { return _impl->mgr; } + test_env_compaction_manager& test_compaction_manager() { return *_impl->cmgr; } reader_concurrency_semaphore& semaphore() { return _impl->semaphore; } db::config& db_config() { return *_impl->db_config; } tmpdir& tempdir() noexcept { return _impl->dir; } From 9b8f03bdb01e8448f9b503402c69dec8a4d9431e Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Nov 2023 13:48:54 +0300 Subject: [PATCH 4/7] table_for_tests: Add const operator-> overload Will be used later in boost transformation lambda Signed-off-by: Pavel Emelyanov --- test/lib/test_services.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/test/lib/test_services.hh b/test/lib/test_services.hh index d88bbdbbca..9bac203212 100644 --- a/test/lib/test_services.hh +++ b/test/lib/test_services.hh @@ -60,6 +60,7 @@ struct table_for_tests { replica::column_family& operator*() { return *_data->cf; } replica::column_family* operator->() { return _data->cf.get(); } + const replica::column_family* operator->() const { return _data->cf.get(); } compaction::table_state& as_table_state() noexcept; From 5b3b8c2176187205434092f7a62ab196ebaaefeb Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Nov 2023 12:29:37 +0300 Subject: [PATCH 5/7] test/sstable_3_x_test: Make use of make_table_for_tests() The compacted_sstable_reader() helper constructs table object and all its "dependencies" by hand. The test_env::make_table_for_tests() helper does the same, so the test code can be simplified. Signed-off-by: Pavel Emelyanov --- test/boost/sstable_3_x_test.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/boost/sstable_3_x_test.cc b/test/boost/sstable_3_x_test.cc index af5011e351..b5684d8b79 100644 --- a/test/boost/sstable_3_x_test.cc +++ b/test/boost/sstable_3_x_test.cc @@ -3013,12 +3013,9 @@ static std::vector open_sstables(test_env& env, schema // Must be called in a seastar thread. static flat_mutation_reader_v2 compacted_sstable_reader(test_env& env, schema_ptr s, sstring table_name, std::vector gen_values) { + env.maybe_start_compaction_manager(false); + auto cf = env.make_table_for_tests(s); auto generations = generations_from_values(gen_values); - auto cm = make_lw_shared(false); - auto cl_stats = make_lw_shared(); - auto tracker = make_lw_shared(); - auto cf = make_lw_shared(s, env.make_table_config(), make_lw_shared(), **cm, env.manager(), *cl_stats, *tracker, nullptr); - cf->mark_ready_for_writes(nullptr); lw_shared_ptr mt = make_lw_shared(s); auto sstables = open_sstables(env, s, format("test/resource/sstables/3.x/uncompressed/{}", table_name), generations); From 731a82869a0321a31c60aa4128ab1a091fd9fbfe Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Nov 2023 13:48:09 +0300 Subject: [PATCH 6/7] test/sstable_compaction_test: Make use of make_table_for_tests() The max_ongoing_compaction_test test case constructs table object by hand. For that it needs tracker, compaction manager and stats. Similarly to previous patch, the test_env::make_table_for_tests() helper does exactly that, so the test case can be simplified as well. Signed-off-by: Pavel Emelyanov --- test/boost/sstable_compaction_test.cc | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/test/boost/sstable_compaction_test.cc b/test/boost/sstable_compaction_test.cc index d86f89a4ec..3f0212c576 100644 --- a/test/boost/sstable_compaction_test.cc +++ b/test/boost/sstable_compaction_test.cc @@ -4111,11 +4111,6 @@ SEASTAR_TEST_CASE(max_ongoing_compaction_test) { return builder.build(); }; - auto cm = compaction_manager_for_testing(); - - auto cl_stats = make_lw_shared(); - auto tracker = make_lw_shared(); - auto next_timestamp = [] (auto step) { using namespace std::chrono; return (gc_clock::now().time_since_epoch() - duration_cast(step)).count(); @@ -4133,7 +4128,7 @@ SEASTAR_TEST_CASE(max_ongoing_compaction_test) { constexpr size_t num_tables = 10; std::vector schemas; - std::vector> tables; + std::vector tables; auto stop_tables = defer([&tables] { for (auto& t : tables) { t->stop().get(); @@ -4151,9 +4146,7 @@ SEASTAR_TEST_CASE(max_ongoing_compaction_test) { cfg.enable_commitlog = false; cfg.enable_incremental_backups = false; - auto cf = make_lw_shared(s, cfg, make_lw_shared(), *cm, env.manager(), *cl_stats, *tracker, nullptr); - cf->start(); - cf->mark_ready_for_writes(nullptr); + auto cf = env.make_table_for_tests(s); tables.push_back(cf); } @@ -4185,24 +4178,25 @@ SEASTAR_TEST_CASE(max_ongoing_compaction_test) { } size_t max_ongoing_compaction = 0; + auto& cm = env.test_compaction_manager().get_compaction_manager(); // wait for submitted jobs to finish. - auto end = [cm, &tables, expected_after] { - return cm->get_stats().pending_tasks == 0 && cm->get_stats().active_tasks == 0 + auto end = [&cm, &tables, expected_after] { + return cm.get_stats().pending_tasks == 0 && cm.get_stats().active_tasks == 0 && boost::algorithm::all_of(tables, [expected_after] (auto& t) { return t->sstables_count() == expected_after; }); }; while (!end()) { - if (!cm->get_stats().pending_tasks && !cm->get_stats().active_tasks) { + if (!cm.get_stats().pending_tasks && !cm.get_stats().active_tasks) { for (auto& t : tables) { if (t->sstables_count()) { t->trigger_compaction(); } } } - max_ongoing_compaction = std::max(cm->get_stats().active_tasks, max_ongoing_compaction); + max_ongoing_compaction = std::max(cm.get_stats().active_tasks, max_ongoing_compaction); yield().get(); } - BOOST_REQUIRE(cm->get_stats().errors == 0); + BOOST_REQUIRE(cm.get_stats().errors == 0); return max_ongoing_compaction; }; From 787c6576fe80eb9c4882388d5955c0203b7c66b9 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Nov 2023 14:08:21 +0300 Subject: [PATCH 7/7] test_services: Ditch compaction_manager_for_testing Now this wrapper is unused, all (both) test cases that needed it were patched to use make_table_for_tests(). Signed-off-by: Pavel Emelyanov --- test/lib/sstable_utils.cc | 16 ---------------- test/lib/sstable_utils.hh | 29 ----------------------------- 2 files changed, 45 deletions(-) diff --git a/test/lib/sstable_utils.cc b/test/lib/sstable_utils.cc index 5e098b8e05..40703468ab 100644 --- a/test/lib/sstable_utils.cc +++ b/test/lib/sstable_utils.cc @@ -145,22 +145,6 @@ future test_env::reusable_sst(schema_ptr schema, sstring dir, ss throw sst_not_found(dir, generation); } -compaction_manager_for_testing::wrapped_compaction_manager::wrapped_compaction_manager(bool enabled) - : cm(tm, compaction_manager::for_testing_tag{}) -{ - if (enabled) { - cm.enable(); - } -} - -// Must run in a seastar thread -compaction_manager_for_testing::wrapped_compaction_manager::~wrapped_compaction_manager() { - if (!tm.abort_source().abort_requested()) { - tm.abort_source().request_abort(); - } - cm.stop().get(); -} - class compaction_manager_test_task : public compaction::compaction_task_executor { sstables::run_id _run_id; noncopyable_function (sstables::compaction_data&)> _job; diff --git a/test/lib/sstable_utils.hh b/test/lib/sstable_utils.hh index 9851e2b5b8..06c7baade5 100644 --- a/test/lib/sstable_utils.hh +++ b/test/lib/sstable_utils.hh @@ -239,35 +239,6 @@ future<> for_each_sstable_version(AsyncAction action) { } // namespace sstables -// Must be used in a seastar thread -class compaction_manager_for_testing { - struct wrapped_compaction_manager { - tasks::task_manager tm; - compaction_manager cm; - explicit wrapped_compaction_manager(bool enabled); - // Must run in a seastar thread - ~wrapped_compaction_manager(); - }; - - lw_shared_ptr _wcm; -public: - explicit compaction_manager_for_testing(bool enabled = true) : _wcm(make_lw_shared(enabled)) {} - - compaction_manager& operator*() noexcept { - return _wcm->cm; - } - const compaction_manager& operator*() const noexcept { - return _wcm->cm; - } - - compaction_manager* operator->() noexcept { - return &_wcm->cm; - } - const compaction_manager* operator->() const noexcept { - return &_wcm->cm; - } -}; - class compaction_manager_test { compaction_manager& _cm; public: