From 0877e7a846e56e3767c2da2c2777fd8a1b7ca6f4 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 31 May 2023 16:24:27 +0300 Subject: [PATCH 1/9] compaction_backlog_tracker: replace_sstables: add FIXME comments about strong exception safety Signed-off-by: Benny Halevy --- compaction/compaction_backlog_manager.hh | 2 ++ compaction/compaction_manager.cc | 1 + compaction/compaction_strategy.cc | 3 +++ 3 files changed, 6 insertions(+) diff --git a/compaction/compaction_backlog_manager.hh b/compaction/compaction_backlog_manager.hh index dc13819b3e..35008ad5e6 100644 --- a/compaction/compaction_backlog_manager.hh +++ b/compaction/compaction_backlog_manager.hh @@ -60,6 +60,7 @@ public: using ongoing_compactions = std::unordered_map; struct impl { + // FIXME: Should provide strong exception safety guarantees virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) = 0; virtual double backlog(const ongoing_writes& ow, const ongoing_compactions& oc) const = 0; virtual ~impl() { } @@ -72,6 +73,7 @@ public: ~compaction_backlog_tracker(); double backlog() const; + // FIXME: Should provide strong exception safety guarantees 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); diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc index bd7cd4860d..bef1c6da4f 100644 --- a/compaction/compaction_manager.cc +++ b/compaction/compaction_manager.cc @@ -1917,6 +1917,7 @@ void compaction_backlog_tracker::replace_sstables(const std::vectorreplace_sstables(filter_and_revert_charges(old_ssts), filter_and_revert_charges(new_ssts)); } catch (...) { diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 9be7910184..591519598d 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -174,6 +174,7 @@ double size_tiered_backlog_tracker::backlog(const compaction_backlog_tracker::on return b > 0 ? b : 0; } +// FIXME: Should provide strong exception safety guarantees void size_tiered_backlog_tracker::replace_sstables(std::vector old_ssts, std::vector new_ssts) { for (auto& sst : old_ssts) { if (sst->data_size() > 0) { @@ -265,6 +266,7 @@ public: return b; } + // FIXME: Should provide strong exception safety guarantees virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override { struct replacement { std::vector old_ssts; @@ -394,6 +396,7 @@ public: return b; } + // FIXME: Should provide strong exception safety guarantees virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override { std::vector l0_old_ssts, l0_new_ssts; for (auto& sst : new_ssts) { From 1a8cc84981729bdbf01f88880f6a837c8a3fc6af Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 31 May 2023 13:33:56 +0300 Subject: [PATCH 2/9] compaction_backlog_tracker: replace_sstables: pass old and new sstables vectors by ref To facilitate rollback on the error handling path, to provide strong exception safety guarantees. Signed-off-by: Benny Halevy --- compaction/compaction_backlog_manager.hh | 2 +- compaction/compaction_manager.cc | 2 +- compaction/compaction_strategy.cc | 10 +++++----- compaction/size_tiered_backlog_tracker.hh | 3 ++- tools/scylla-sstable.cc | 2 +- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/compaction/compaction_backlog_manager.hh b/compaction/compaction_backlog_manager.hh index 35008ad5e6..13ad1724d3 100644 --- a/compaction/compaction_backlog_manager.hh +++ b/compaction/compaction_backlog_manager.hh @@ -61,7 +61,7 @@ public: struct impl { // FIXME: Should provide strong exception safety guarantees - virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) = 0; + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) = 0; virtual double backlog(const ongoing_writes& ow, const ongoing_compactions& oc) const = 0; virtual ~impl() { } }; diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc index bef1c6da4f..bf964ac48e 100644 --- a/compaction/compaction_manager.cc +++ b/compaction/compaction_manager.cc @@ -274,7 +274,7 @@ private: virtual double backlog(const compaction_backlog_tracker::ongoing_writes& ow, const compaction_backlog_tracker::ongoing_compactions& oc) const override { return _added_backlog * _available_memory; } - virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override {} + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override {} }; compaction::compaction_state& compaction_manager::get_compaction_state(table_state* t) { diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 591519598d..9390c63f2c 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -175,7 +175,7 @@ double size_tiered_backlog_tracker::backlog(const compaction_backlog_tracker::on } // FIXME: Should provide strong exception safety guarantees -void size_tiered_backlog_tracker::replace_sstables(std::vector old_ssts, std::vector new_ssts) { +void size_tiered_backlog_tracker::replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) { for (auto& sst : old_ssts) { if (sst->data_size() > 0) { _total_bytes -= sst->data_size(); @@ -267,7 +267,7 @@ public: } // FIXME: Should provide strong exception safety guarantees - virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override { + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override { struct replacement { std::vector old_ssts; std::vector new_ssts; @@ -397,7 +397,7 @@ public: } // FIXME: Should provide strong exception safety guarantees - virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override { + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override { std::vector l0_old_ssts, l0_new_ssts; for (auto& sst : new_ssts) { auto level = sst->get_sstable_level(); @@ -423,14 +423,14 @@ struct unimplemented_backlog_tracker final : public compaction_backlog_tracker:: virtual double backlog(const compaction_backlog_tracker::ongoing_writes& ow, const compaction_backlog_tracker::ongoing_compactions& oc) const override { return compaction_controller::disable_backlog; } - virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override {} + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override {} }; struct null_backlog_tracker final : public compaction_backlog_tracker::impl { virtual double backlog(const compaction_backlog_tracker::ongoing_writes& ow, const compaction_backlog_tracker::ongoing_compactions& oc) const override { return 0; } - virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override {} + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override {} }; // diff --git a/compaction/size_tiered_backlog_tracker.hh b/compaction/size_tiered_backlog_tracker.hh index 3165f3bf64..d77da53d6f 100644 --- a/compaction/size_tiered_backlog_tracker.hh +++ b/compaction/size_tiered_backlog_tracker.hh @@ -90,7 +90,8 @@ public: // Removing could be the result of a failure of an in progress write, successful finish of a // compaction, or some one-off operation, like drop - virtual void replace_sstables(std::vector old_ssts, std::vector new_ssts) override; + // FIXME: Should provide strong exception safety guarantees + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override; int64_t total_bytes() const { return _total_bytes; diff --git a/tools/scylla-sstable.cc b/tools/scylla-sstable.cc index 1cf85b3362..220810a5ac 100644 --- a/tools/scylla-sstable.cc +++ b/tools/scylla-sstable.cc @@ -916,7 +916,7 @@ void consume_sstables(schema_ptr schema, reader_permit permit, std::vector old_ssts, std::vector new_ssts) override { } + virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override { } virtual double backlog(const compaction_backlog_tracker::ongoing_writes& ow, const compaction_backlog_tracker::ongoing_compactions& oc) const override { return 0.0; } }; From bf69584ccc0008dd923185a0aea880a2f7b7d9cf Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 6 Jun 2023 12:21:30 +0300 Subject: [PATCH 3/9] size_tiered_backlog_tracker: update_sstables: update total_bytes only if set changed Although replace_sstables is supposed to be called only once per {old_ssts, new_ssts} it is safer to update `_total_bytes` with `sst->data_size()` only if the sst was inserted/erased successfully. Otherwise _total_bytes may go out of sync with the contents of _all. That said, the next step should be to refer to the compaction_group's main sstable set directly rather than maintaining a "shadow" set in the tracker. Signed-off-by: Benny Halevy --- compaction/compaction_strategy.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 9390c63f2c..1c658b4c20 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -178,14 +178,18 @@ double size_tiered_backlog_tracker::backlog(const compaction_backlog_tracker::on void size_tiered_backlog_tracker::replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) { for (auto& sst : old_ssts) { if (sst->data_size() > 0) { - _total_bytes -= sst->data_size(); - _all.erase(sst); + auto erased = _all.erase(sst); + if (erased) { + _total_bytes -= sst->data_size(); + } } } for (auto& sst : new_ssts) { if (sst->data_size() > 0) { - _total_bytes += sst->data_size(); - _all.insert(std::move(sst)); + auto [_, inserted] = _all.insert(sst); + if (inserted) { + _total_bytes += sst->data_size(); + } } } refresh_sstables_backlog_contribution(); From 5d6c2b0d12053bec8175645c18f4221a39b2f778 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 31 May 2023 15:35:50 +0300 Subject: [PATCH 4/9] size_tiered_backlog_tracker: define struct sstables_backlog_contribution Encapsulate the contribution-related members in struct contribution, to be used for strong exception safety. Signed-off-by: Benny Halevy --- compaction/compaction_strategy.cc | 16 +++++++++------- compaction/size_tiered_backlog_tracker.hh | 9 +++++++-- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 1c658b4c20..03250cfd68 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -109,7 +109,7 @@ size_tiered_backlog_tracker::compacted_backlog(const compaction_backlog_tracker: // A SSTable being compacted may not contribute to backlog if compaction strategy decided // to perform a low-efficiency compaction when system is under little load, or when user // performs major even though strategy is completely satisfied - if (!_sstables_contributing_backlog.contains(crp.first)) { + if (!_contrib.sstables.contains(crp.first)) { continue; } auto compacted = crp.second->compacted(); @@ -119,9 +119,9 @@ size_tiered_backlog_tracker::compacted_backlog(const compaction_backlog_tracker: return in; } +// Provides strong exception safety guarantees. void size_tiered_backlog_tracker::refresh_sstables_backlog_contribution() { - _sstables_backlog_contribution = 0.0f; - _sstables_contributing_backlog = {}; + sstables_backlog_contribution contrib; if (_all.empty()) { return; } @@ -140,18 +140,20 @@ void size_tiered_backlog_tracker::refresh_sstables_backlog_contribution() { if (!size_tiered_compaction_strategy::is_bucket_interesting(bucket, threshold)) { continue; } - _sstables_backlog_contribution += boost::accumulate(bucket | boost::adaptors::transformed([this] (const shared_sstable& sst) -> double { + contrib.value += boost::accumulate(bucket | boost::adaptors::transformed([this] (const shared_sstable& sst) -> double { return sst->data_size() * log4(sst->data_size()); }), double(0.0f)); // Controller is disabled if exception is caught during add / remove calls, so not making any effort to make this exception safe - _sstables_contributing_backlog.insert(bucket.begin(), bucket.end()); + contrib.sstables.insert(bucket.begin(), bucket.end()); } + + _contrib = std::move(contrib); } double size_tiered_backlog_tracker::backlog(const compaction_backlog_tracker::ongoing_writes& ow, const compaction_backlog_tracker::ongoing_compactions& oc) const { inflight_component compacted = compacted_backlog(oc); - auto total_backlog_bytes = boost::accumulate(_sstables_contributing_backlog | boost::adaptors::transformed(std::mem_fn(&sstables::sstable::data_size)), uint64_t(0)); + auto total_backlog_bytes = boost::accumulate(_contrib.sstables | boost::adaptors::transformed(std::mem_fn(&sstables::sstable::data_size)), uint64_t(0)); // Bail out if effective backlog is zero, which happens in a small window where ongoing compaction exhausted // input files but is still sealing output files or doing managerial stuff like updating history table @@ -168,7 +170,7 @@ double size_tiered_backlog_tracker::backlog(const compaction_backlog_tracker::on auto effective_backlog_bytes = total_backlog_bytes - compacted.total_bytes; // Sum of (Si - Ci) * log (Si) for all SSTables contributing backlog - auto sstables_contribution = _sstables_backlog_contribution - compacted.contribution; + auto sstables_contribution = _contrib.value - compacted.contribution; // This is subtracting ((Si - Ci) * log (Si)) from ((Si - Ci) * log(T)), yielding the final backlog auto b = (effective_backlog_bytes * log4(_total_bytes)) - sstables_contribution; return b > 0 ? b : 0; diff --git a/compaction/size_tiered_backlog_tracker.hh b/compaction/size_tiered_backlog_tracker.hh index d77da53d6f..bc0cdcebc1 100644 --- a/compaction/size_tiered_backlog_tracker.hh +++ b/compaction/size_tiered_backlog_tracker.hh @@ -64,10 +64,14 @@ // certain point in time, whose size is the amount of bytes currently written. So all we need // to do is keep track of them too, and add the current estimate to the static part of (4). class size_tiered_backlog_tracker final : public compaction_backlog_tracker::impl { + struct sstables_backlog_contribution { + double value = 0.0f; + std::unordered_set sstables; + }; + sstables::size_tiered_compaction_strategy_options _stcs_options; int64_t _total_bytes = 0; - double _sstables_backlog_contribution = 0.0f; - std::unordered_set _sstables_contributing_backlog; + sstables_backlog_contribution _contrib; std::unordered_set _all; struct inflight_component { @@ -82,6 +86,7 @@ class size_tiered_backlog_tracker final : public compaction_backlog_tracker::imp return log(x) * inv_log_4; } + // Provides strong exception safety guarantees. void refresh_sstables_backlog_contribution(); public: size_tiered_backlog_tracker(sstables::size_tiered_compaction_strategy_options stcs_options) : _stcs_options(stcs_options) {} From 4e5bfe2c184ffcca135ef5d2f9d72dbd645405e8 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 13 Jun 2023 12:00:25 +0300 Subject: [PATCH 5/9] size_tiered_backlog_tracker: make log4 helper static It is completely generic. Signed-off-by: Benny Halevy --- compaction/compaction_strategy.cc | 2 +- compaction/size_tiered_backlog_tracker.hh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 03250cfd68..1f773351d4 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -140,7 +140,7 @@ void size_tiered_backlog_tracker::refresh_sstables_backlog_contribution() { if (!size_tiered_compaction_strategy::is_bucket_interesting(bucket, threshold)) { continue; } - contrib.value += boost::accumulate(bucket | boost::adaptors::transformed([this] (const shared_sstable& sst) -> double { + contrib.value += boost::accumulate(bucket | boost::adaptors::transformed([] (const shared_sstable& sst) -> double { return sst->data_size() * log4(sst->data_size()); }), double(0.0f)); // Controller is disabled if exception is caught during add / remove calls, so not making any effort to make this exception safe diff --git a/compaction/size_tiered_backlog_tracker.hh b/compaction/size_tiered_backlog_tracker.hh index bc0cdcebc1..5a4137525a 100644 --- a/compaction/size_tiered_backlog_tracker.hh +++ b/compaction/size_tiered_backlog_tracker.hh @@ -81,7 +81,7 @@ class size_tiered_backlog_tracker final : public compaction_backlog_tracker::imp inflight_component compacted_backlog(const compaction_backlog_tracker::ongoing_compactions& ongoing_compactions) const; - double log4(double x) const { + static double log4(double x) { double inv_log_4 = 1.0f / std::log(4); return log(x) * inv_log_4; } From 054a031504d4b2159915af286e04d17399c34647 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 6 Jun 2023 22:32:39 +0300 Subject: [PATCH 6/9] size_tiered_backlog_tracker: provide static calculate_sstables_backlog_contribution Instead of providing refresh_sstables_backlog_contribution that updates the tracker in place, provide a static function calculate_sstables_backlog_contribution that doesn't change the tracker state to facilitate exception safety in the next patch. Signed-off-by: Benny Halevy --- compaction/compaction_strategy.cc | 15 ++++++++------- compaction/size_tiered_backlog_tracker.hh | 3 +-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 1f773351d4..70e0ff4ad2 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -12,6 +12,7 @@ #include #include #include +#include "sstables/shared_sstable.hh" #include "sstables/sstables.hh" #include "compaction.hh" #include "compaction_strategy.hh" @@ -120,10 +121,10 @@ size_tiered_backlog_tracker::compacted_backlog(const compaction_backlog_tracker: } // Provides strong exception safety guarantees. -void size_tiered_backlog_tracker::refresh_sstables_backlog_contribution() { +size_tiered_backlog_tracker::sstables_backlog_contribution size_tiered_backlog_tracker::calculate_sstables_backlog_contribution(const std::vector& all, const sstables::size_tiered_compaction_strategy_options& stcs_options) { sstables_backlog_contribution contrib; - if (_all.empty()) { - return; + if (all.empty()) { + return contrib; } using namespace sstables; @@ -133,10 +134,10 @@ void size_tiered_backlog_tracker::refresh_sstables_backlog_contribution() { // in efficient jobs acting more aggressive than they really have to. // TODO: potentially switch to compaction manager's fan-in threshold, so to account for the dynamic // fan-in threshold behavior. - const auto& newest_sst = std::ranges::max(_all, std::less(), std::mem_fn(&sstable::generation)); + const auto& newest_sst = std::ranges::max(all, std::less(), std::mem_fn(&sstable::generation)); auto threshold = newest_sst->get_schema()->min_compaction_threshold(); - for (auto& bucket : size_tiered_compaction_strategy::get_buckets(boost::copy_range>(_all), _stcs_options)) { + for (auto& bucket : size_tiered_compaction_strategy::get_buckets(all, stcs_options)) { if (!size_tiered_compaction_strategy::is_bucket_interesting(bucket, threshold)) { continue; } @@ -147,7 +148,7 @@ void size_tiered_backlog_tracker::refresh_sstables_backlog_contribution() { contrib.sstables.insert(bucket.begin(), bucket.end()); } - _contrib = std::move(contrib); + return contrib; } double size_tiered_backlog_tracker::backlog(const compaction_backlog_tracker::ongoing_writes& ow, const compaction_backlog_tracker::ongoing_compactions& oc) const { @@ -194,7 +195,7 @@ void size_tiered_backlog_tracker::replace_sstables(const std::vector>(_all), _stcs_options); } namespace sstables { diff --git a/compaction/size_tiered_backlog_tracker.hh b/compaction/size_tiered_backlog_tracker.hh index 5a4137525a..80b81f1fb4 100644 --- a/compaction/size_tiered_backlog_tracker.hh +++ b/compaction/size_tiered_backlog_tracker.hh @@ -86,8 +86,7 @@ class size_tiered_backlog_tracker final : public compaction_backlog_tracker::imp return log(x) * inv_log_4; } - // Provides strong exception safety guarantees. - void refresh_sstables_backlog_contribution(); + static sstables_backlog_contribution calculate_sstables_backlog_contribution(const std::vector& all, const sstables::size_tiered_compaction_strategy_options& stcs_options); public: size_tiered_backlog_tracker(sstables::size_tiered_compaction_strategy_options stcs_options) : _stcs_options(stcs_options) {} From 635c564a9d9a75c31e42d20d0929741e3ead5504 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 31 May 2023 15:40:20 +0300 Subject: [PATCH 7/9] size_tiered_backlog_tracker: replace_sstables: provide strong exception safety guarantees By making all changes on temporary variables and eventually moving them back into the tracker members in a noexcept block the function can safely throw until the changes are committed. Signed-off-by: Benny Halevy --- compaction/compaction_strategy.cc | 22 ++++++++++++++++------ compaction/size_tiered_backlog_tracker.hh | 2 +- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 70e0ff4ad2..4cd1c28b7d 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -177,25 +177,35 @@ double size_tiered_backlog_tracker::backlog(const compaction_backlog_tracker::on return b > 0 ? b : 0; } -// FIXME: Should provide strong exception safety guarantees +// Provides strong exception safety guarantees. void size_tiered_backlog_tracker::replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) { + auto tmp_all = _all; + auto tmp_total_bytes = _total_bytes; + tmp_all.reserve(_all.size() + new_ssts.size()); + for (auto& sst : old_ssts) { if (sst->data_size() > 0) { - auto erased = _all.erase(sst); + auto erased = tmp_all.erase(sst); if (erased) { - _total_bytes -= sst->data_size(); + tmp_total_bytes -= sst->data_size(); } } } for (auto& sst : new_ssts) { if (sst->data_size() > 0) { - auto [_, inserted] = _all.insert(sst); + auto [_, inserted] = tmp_all.insert(sst); if (inserted) { - _total_bytes += sst->data_size(); + tmp_total_bytes += sst->data_size(); } } } - _contrib = calculate_sstables_backlog_contribution(boost::copy_range>(_all), _stcs_options); + auto tmp_contrib = calculate_sstables_backlog_contribution(boost::copy_range>(tmp_all), _stcs_options); + + std::invoke([&] () noexcept { + _all = std::move(tmp_all); + _total_bytes = tmp_total_bytes; + _contrib = std::move(tmp_contrib); + }); } namespace sstables { diff --git a/compaction/size_tiered_backlog_tracker.hh b/compaction/size_tiered_backlog_tracker.hh index 80b81f1fb4..61f2123c7a 100644 --- a/compaction/size_tiered_backlog_tracker.hh +++ b/compaction/size_tiered_backlog_tracker.hh @@ -94,7 +94,7 @@ public: // Removing could be the result of a failure of an in progress write, successful finish of a // compaction, or some one-off operation, like drop - // FIXME: Should provide strong exception safety guarantees + // Provides strong exception safety guarantees. virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override; int64_t total_bytes() const { From 39d4b548fc83c47830bcbf31bd87c0cf71ca464d Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 31 May 2023 16:03:51 +0300 Subject: [PATCH 8/9] time_window_backlog_tracker: replace_sstables: provide strong exception safety guarantees Modify a temporary copy of the `_windows` map and move-assign it back atomically when done. Signed-off-by: Benny Halevy --- compaction/compaction_strategy.cc | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 4cd1c28b7d..9206c6db75 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -12,6 +12,7 @@ #include #include #include +#include "seastar/core/on_internal_error.hh" #include "sstables/shared_sstable.hh" #include "sstables/sstables.hh" #include "compaction.hh" @@ -283,24 +284,25 @@ public: return b; } - // FIXME: Should provide strong exception safety guarantees + // Provides strong exception safety guarantees virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override { struct replacement { std::vector old_ssts; std::vector new_ssts; }; std::unordered_map per_window_replacement; + auto tmp_windows = _windows; for (auto& sst : new_ssts) { auto bound = lower_bound_of(sst->get_stats_metadata().max_timestamp); - if (!_windows.contains(bound)) { - _windows.emplace(bound, size_tiered_backlog_tracker(_stcs_options)); + if (!tmp_windows.contains(bound)) { + tmp_windows.emplace(bound, size_tiered_backlog_tracker(_stcs_options)); } per_window_replacement[bound].new_ssts.push_back(std::move(sst)); } for (auto& sst : old_ssts) { auto bound = lower_bound_of(sst->get_stats_metadata().max_timestamp); - if (_windows.contains(bound)) { + if (tmp_windows.contains(bound)) { per_window_replacement[bound].old_ssts.push_back(std::move(sst)); } } @@ -308,12 +310,20 @@ public: for (auto& [bound, r] : per_window_replacement) { // All windows must exist here, as windows are created for new files and will // remain alive as long as there's a single file in them - auto& w = _windows.at(bound); - w.replace_sstables(std::move(r.old_ssts), std::move(r.new_ssts)); + auto it = tmp_windows.find(bound); + if (it == tmp_windows.end()) { + on_internal_error(clogger, fmt::format("window for bound {} not found", bound)); + } + auto& w = it->second; + w.replace_sstables(r.old_ssts, r.new_ssts); if (w.total_bytes() <= 0) { - _windows.erase(bound); + tmp_windows.erase(bound); } } + + std::invoke([&] () noexcept { + _windows = std::move(tmp_windows); + }); } }; From 5710ec55c2b510dfc320898967b984af2cbad16f Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 31 May 2023 16:14:08 +0300 Subject: [PATCH 9/9] leveled_compaction_backlog_tracker: replace_sstables: provide strong exception safety guarantees Modify a temporary copy of `_size_per_level` and apply it back only when done. Signed-off-by: Benny Halevy --- compaction/compaction_strategy.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/compaction/compaction_strategy.cc b/compaction/compaction_strategy.cc index 9206c6db75..1d5d0104b9 100644 --- a/compaction/compaction_strategy.cc +++ b/compaction/compaction_strategy.cc @@ -423,26 +423,31 @@ public: return b; } - // FIXME: Should provide strong exception safety guarantees + // Provides strong exception safety guarantees virtual void replace_sstables(const std::vector& old_ssts, const std::vector& new_ssts) override { + auto tmp_size_per_level = _size_per_level; std::vector l0_old_ssts, l0_new_ssts; for (auto& sst : new_ssts) { auto level = sst->get_sstable_level(); - _size_per_level[level] += sst->data_size(); + tmp_size_per_level[level] += sst->data_size(); if (level == 0) { l0_new_ssts.push_back(std::move(sst)); } } for (auto& sst : old_ssts) { auto level = sst->get_sstable_level(); - _size_per_level[level] -= sst->data_size(); + tmp_size_per_level[level] -= sst->data_size(); if (level == 0) { l0_old_ssts.push_back(std::move(sst)); } } if (l0_old_ssts.size() || l0_new_ssts.size()) { + // stcs replace_sstables guarantees strong exception safety _l0_scts.replace_sstables(std::move(l0_old_ssts), std::move(l0_new_ssts)); } + std::invoke([&] () noexcept { + _size_per_level = std::move(tmp_size_per_level); + }); } };