From c8768f9102827b17491f2a9e232a4beb95fea6ff Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 10 Dec 2023 09:23:13 +0200 Subject: [PATCH 1/4] table: stop: allow compactions to be stopped while closing async_gate To make sure a table object is kept valid throughout the lifetime of compaction a following patch will enter the table's _async_gate when the compaction task starts. This change defers awaiting the gate.close future till after stopping ongoing compaction so that closing the gate will prevent starting new compactions while ongoing compaction can be stopped and finally awaiting the close() future will wait for them to unwind and exit the gate after being stopped. Signed-off-by: Benny Halevy --- replica/table.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/replica/table.cc b/replica/table.cc index 2488a507b7..af3560c8f5 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -1073,10 +1073,13 @@ table::stop() { if (_async_gate.is_closed()) { co_return; } - co_await _async_gate.close(); + // Allow `compaction_group::stop` to stop ongoing compactions + // while they may still hold the table _async_gate + auto gate_closed_fut = _async_gate.close(); co_await await_pending_ops(); co_await parallel_foreach_compaction_group(std::mem_fn(&compaction_group::stop)); co_await _sstable_deletion_gate.close(); + co_await std::move(gate_closed_fut); co_await get_row_cache().invalidate(row_cache::external_updater([this] { for (const compaction_group_ptr& cg : compaction_groups()) { cg->clear_sstables(); From cddcf3ad0cda3685430becefd6f40185371d136f Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 10 Dec 2023 09:18:03 +0200 Subject: [PATCH 2/4] table: add table_holder and hold method A smart pointer that guards the table object while it's being used by async functions. Signed-off-by: Benny Halevy --- replica/database.hh | 30 ++++++++++++++++++++++++++++++ replica/table.cc | 5 +++++ 2 files changed, 35 insertions(+) diff --git a/replica/database.hh b/replica/database.hh index bff06cbd54..b0e3e12938 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -361,6 +361,31 @@ struct table_stats { using storage_options = data_dictionary::storage_options; +// Smart table pointer that guards the table object +// while it's being accessed asynchronously +class table_holder { + gate::holder _holder; + lw_shared_ptr _table_ptr; +public: + explicit table_holder(table&); + + const table& operator*() const noexcept { + return *_table_ptr; + } + + table& operator*() noexcept { + return *_table_ptr; + } + + const table* operator->() const noexcept { + return _table_ptr.operator->(); + } + + table* operator->() noexcept { + return _table_ptr.operator->(); + } +}; + class table : public enable_lw_shared_from_this
, public weakly_referencable
{ public: @@ -783,6 +808,11 @@ public: table(column_family&&) = delete; // 'this' is being captured during construction ~table(); + + table_holder hold() { + return table_holder(*this); + } + const schema_ptr& schema() const { return _schema; } void set_schema(schema_ptr); db::commitlog* commitlog() const; diff --git a/replica/table.cc b/replica/table.cc index af3560c8f5..0b86cdc8ff 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -67,6 +67,11 @@ static seastar::metrics::label keyspace_label("ks"); using namespace std::chrono_literals; +table_holder::table_holder(table& t) + : _holder(t.async_gate()) + , _table_ptr(t.shared_from_this()) +{ } + void table::update_sstables_known_generation(sstables::generation_type generation) { auto gen = generation ? generation.as_int() : 0; if (_sstable_generation_generator) { From 92c718c60aa585f5fd0adef8f0cf5d2a9cef8997 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 10 Dec 2023 09:26:55 +0200 Subject: [PATCH 3/4] compaction: run_on_table: hold table To ensure the table will not be dropped while the compaction task is ongoing. Signed-off-by: Benny Halevy --- compaction/task_manager_module.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compaction/task_manager_module.cc b/compaction/task_manager_module.cc index 6d7e60241b..0569a3dc9c 100644 --- a/compaction/task_manager_module.cc +++ b/compaction/task_manager_module.cc @@ -193,7 +193,9 @@ future<> run_on_table(sstring op, replica::database& db, std::string keyspace, t std::exception_ptr ex; tasks::tmlogger.debug("Starting {} on {}.{}", op, keyspace, ti.name); try { - co_await func(db.find_column_family(ti.id)); + auto& t = db.find_column_family(ti.id); + auto holder = t.hold(); + co_await func(t); } catch (const replica::no_such_column_family& e) { tasks::tmlogger.warn("Skipping {} of {}.{}: {}", op, keyspace, ti.name, e.what()); } catch (...) { From 7843025a53fc808120c4f28e578a59817214ce04 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 10 Dec 2023 11:31:11 +0200 Subject: [PATCH 4/4] compaction: run_on_table: skip compaction also on gate_closed_exception Similar to the no_such_column_family error, gate_closed_exception indicates that the table is stopped and we should skip compaction on it gracefully. Fixes #16305 Signed-off-by: Benny Halevy --- compaction/task_manager_module.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compaction/task_manager_module.cc b/compaction/task_manager_module.cc index 0569a3dc9c..30676171fe 100644 --- a/compaction/task_manager_module.cc +++ b/compaction/task_manager_module.cc @@ -198,6 +198,8 @@ future<> run_on_table(sstring op, replica::database& db, std::string keyspace, t co_await func(t); } catch (const replica::no_such_column_family& e) { tasks::tmlogger.warn("Skipping {} of {}.{}: {}", op, keyspace, ti.name, e.what()); + } catch (const gate_closed_exception& e) { + tasks::tmlogger.warn("Skipping {} of {}.{}: {}", op, keyspace, ti.name, e.what()); } catch (...) { ex = std::current_exception(); tasks::tmlogger.error("Failed {} of {}.{}: {}", op, keyspace, ti.name, ex);