From 244efddb224c95dd2410aeeea1e39b289c3f88bf Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Fri, 4 Nov 2022 15:04:04 -0300 Subject: [PATCH] Fix exception safety when transferring ongoing charges to new backlog tracker When setting a new strategy, the charges of old tracker is transferred to the new one. The problem is that we're not reverting changes if exception is triggered before the new strategy is successfully set. To fix this exception safety issue, let's copy the charges instead of moving them. If exception is triggered, the old tracker is still the one used and remain intact. Signed-off-by: Raphael S. Carvalho --- compaction/compaction_backlog_manager.hh | 2 +- compaction/compaction_manager.cc | 4 +--- replica/table.cc | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/compaction/compaction_backlog_manager.hh b/compaction/compaction_backlog_manager.hh index b996f7bc28..b6cf78dc6f 100644 --- a/compaction/compaction_backlog_manager.hh +++ b/compaction/compaction_backlog_manager.hh @@ -74,7 +74,7 @@ public: void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts); void register_partially_written_sstable(sstables::shared_sstable sst, backlog_write_progress_manager& wp); void register_compacting_sstable(sstables::shared_sstable sst, backlog_read_progress_manager& rp); - void transfer_ongoing_charges(compaction_backlog_tracker& new_bt, bool move_read_charges = true); + void copy_ongoing_charges(compaction_backlog_tracker& new_bt, bool move_read_charges = true) const; void revert_charges(sstables::shared_sstable sst); void disable() { diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc index 18cdba6151..613081f8f2 100644 --- a/compaction/compaction_manager.cc +++ b/compaction/compaction_manager.cc @@ -1761,7 +1761,7 @@ void compaction_backlog_tracker::register_compacting_sstable(sstables::shared_ss } } -void compaction_backlog_tracker::transfer_ongoing_charges(compaction_backlog_tracker& new_bt, bool move_read_charges) { +void compaction_backlog_tracker::copy_ongoing_charges(compaction_backlog_tracker& new_bt, bool move_read_charges) const { for (auto&& w : _ongoing_writes) { new_bt.register_partially_written_sstable(w.first, *w.second); } @@ -1771,8 +1771,6 @@ void compaction_backlog_tracker::transfer_ongoing_charges(compaction_backlog_tra new_bt.register_compacting_sstable(w.first, *w.second); } } - _ongoing_writes = {}; - _ongoing_compactions = {}; } void compaction_backlog_tracker::revert_charges(sstables::shared_sstable sst) { diff --git a/replica/table.cc b/replica/table.cc index 0c18e805d8..5a8f4bb8dc 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -1160,7 +1160,7 @@ void table::set_compaction_strategy(sstables::compaction_strategy_type strategy) // FIXME: decouple backlog_tracker from compaction_strategy, so each group can have its own tracker. _compaction_manager.register_backlog_tracker(new_cs.get_backlog_tracker()); auto move_read_charges = new_cs.type() == _compaction_strategy.type(); - _compaction_strategy.get_backlog_tracker().transfer_ongoing_charges(new_cs.get_backlog_tracker(), move_read_charges); + _compaction_strategy.get_backlog_tracker().copy_ongoing_charges(new_cs.get_backlog_tracker(), move_read_charges); compaction_group& cg = *_compaction_group; auto new_sstables = make_lw_shared(new_cs.make_sstable_set(_schema));