From a1ab46d54d3fa8554070f4bbd2183e4c19d87ee1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Thu, 9 Jan 2025 09:55:31 -0500 Subject: [PATCH] replica: remove noexcept from token -> tablet resolution path The methods to resolve a key/token/range to a table are all noexcept. Yet the method below all of these, `storage_group_for_id()` can throw. This means that if due to any mistake a tablet without local replica is attempted to be looked up, it will result in a crash, as the exception bubbles up into the noexcept methods. There is no value in pretending that looking up the tablet replica is noexcept, remove the noexcept specifiers so that any bad lookup only fails the operation at hand and doesn't crash the node. This is especially relevant to replace, which still has a window where writes can arrive for tablets that don't (yet) have a local replica. Currently, this results in a crash. After this patch, this will only fail the writes and the replace can move on. Fixes: #21480 Closes scylladb/scylladb#22251 (cherry picked from commit 55963f8f795db2a3233f9397707e95356c347885) Closes scylladb/scylladb#22378 --- replica/compaction_group.hh | 8 ++++---- replica/database.hh | 10 +++++----- replica/table.cc | 32 ++++++++++++++++---------------- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/replica/compaction_group.hh b/replica/compaction_group.hh index 4e587c5096..299ae4e434 100644 --- a/replica/compaction_group.hh +++ b/replica/compaction_group.hh @@ -320,13 +320,13 @@ public: // new tablet replica is allocated). virtual future<> update_effective_replication_map(const locator::effective_replication_map& erm, noncopyable_function refresh_mutation_source) = 0; - virtual compaction_group& compaction_group_for_token(dht::token token) const noexcept = 0; + virtual compaction_group& compaction_group_for_token(dht::token token) const = 0; virtual utils::chunked_vector compaction_groups_for_token_range(dht::token_range tr) const = 0; - virtual compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const noexcept = 0; - virtual compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const noexcept = 0; + virtual compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const = 0; + virtual compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const = 0; virtual size_t log2_storage_groups() const = 0; - virtual storage_group& storage_group_for_token(dht::token) const noexcept = 0; + virtual storage_group& storage_group_for_token(dht::token) const = 0; virtual locator::table_load_stats table_load_stats(std::function tablet_filter) const noexcept = 0; virtual bool all_storage_groups_split() = 0; diff --git a/replica/database.hh b/replica/database.hh index edbc4f872c..b43ede41c5 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -602,19 +602,19 @@ private: future<> handle_tablet_split_completion(size_t old_tablet_count, const locator::tablet_map& new_tmap); // Select a storage group from a given token. - storage_group& storage_group_for_token(dht::token token) const noexcept; + storage_group& storage_group_for_token(dht::token token) const; storage_group& storage_group_for_id(size_t i) const; std::unique_ptr make_storage_group_manager(); - compaction_group* get_compaction_group(size_t id) const noexcept; + compaction_group* get_compaction_group(size_t id) const; // Select a compaction group from a given token. - compaction_group& compaction_group_for_token(dht::token token) const noexcept; + compaction_group& compaction_group_for_token(dht::token token) const; // Return compaction groups, present in this shard, that own a particular token range. utils::chunked_vector compaction_groups_for_token_range(dht::token_range tr) const; // Select a compaction group from a given key. - compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const noexcept; + compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const; // Select a compaction group from a given sstable based on its token range. - compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const noexcept; + compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const; // Safely iterate through compaction groups, while performing async operations on them. future<> parallel_foreach_compaction_group(std::function(compaction_group&)> action); void for_each_compaction_group(std::function action); diff --git a/replica/table.cc b/replica/table.cc index 16a880594b..30559b5530 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -660,7 +660,7 @@ public: future<> update_effective_replication_map(const locator::effective_replication_map& erm, noncopyable_function refresh_mutation_source) override { return make_ready_future(); } - compaction_group& compaction_group_for_token(dht::token token) const noexcept override { + compaction_group& compaction_group_for_token(dht::token token) const override { return get_compaction_group(); } utils::chunked_vector compaction_groups_for_token_range(dht::token_range tr) const override { @@ -668,16 +668,16 @@ public: ret.push_back(&get_compaction_group()); return ret; } - compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const noexcept override { + compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const override { return get_compaction_group(); } - compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const noexcept override { + compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const override { return get_compaction_group(); } size_t log2_storage_groups() const override { return 0; } - storage_group& storage_group_for_token(dht::token token) const noexcept override { + storage_group& storage_group_for_token(dht::token token) const override { return *_single_sg; } @@ -785,15 +785,15 @@ public: future<> update_effective_replication_map(const locator::effective_replication_map& erm, noncopyable_function refresh_mutation_source) override; - compaction_group& compaction_group_for_token(dht::token token) const noexcept override; + compaction_group& compaction_group_for_token(dht::token token) const override; utils::chunked_vector compaction_groups_for_token_range(dht::token_range tr) const override; - compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const noexcept override; - compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const noexcept override; + compaction_group& compaction_group_for_key(partition_key_view key, const schema_ptr& s) const override; + compaction_group& compaction_group_for_sstable(const sstables::shared_sstable& sst) const override; size_t log2_storage_groups() const override { return log2ceil(tablet_map().tablet_count()); } - storage_group& storage_group_for_token(dht::token token) const noexcept override { + storage_group& storage_group_for_token(dht::token token) const override { return storage_group_for_id(storage_group_of(token).first); } @@ -1004,11 +1004,11 @@ std::unique_ptr table::make_storage_group_manager() { return ret; } -compaction_group* table::get_compaction_group(size_t id) const noexcept { +compaction_group* table::get_compaction_group(size_t id) const { return storage_group_for_id(id).main_compaction_group().get(); } -storage_group& table::storage_group_for_token(dht::token token) const noexcept { +storage_group& table::storage_group_for_token(dht::token token) const { return _sg_manager->storage_group_for_token(token); } @@ -1016,13 +1016,13 @@ storage_group& table::storage_group_for_id(size_t i) const { return _sg_manager->storage_group_for_id(_schema, i); } -compaction_group& tablet_storage_group_manager::compaction_group_for_token(dht::token token) const noexcept { +compaction_group& tablet_storage_group_manager::compaction_group_for_token(dht::token token) const { auto [idx, range_side] = storage_group_of(token); auto& sg = storage_group_for_id(idx); return *sg.select_compaction_group(range_side); } -compaction_group& table::compaction_group_for_token(dht::token token) const noexcept { +compaction_group& table::compaction_group_for_token(dht::token token) const { return _sg_manager->compaction_group_for_token(token); } @@ -1053,15 +1053,15 @@ utils::chunked_vector table::compaction_groups_for_token_rang return _sg_manager->compaction_groups_for_token_range(tr); } -compaction_group& tablet_storage_group_manager::compaction_group_for_key(partition_key_view key, const schema_ptr& s) const noexcept { +compaction_group& tablet_storage_group_manager::compaction_group_for_key(partition_key_view key, const schema_ptr& s) const { return compaction_group_for_token(dht::get_token(*s, key)); } -compaction_group& table::compaction_group_for_key(partition_key_view key, const schema_ptr& s) const noexcept { +compaction_group& table::compaction_group_for_key(partition_key_view key, const schema_ptr& s) const { return _sg_manager->compaction_group_for_key(key, s); } -compaction_group& tablet_storage_group_manager::compaction_group_for_sstable(const sstables::shared_sstable& sst) const noexcept { +compaction_group& tablet_storage_group_manager::compaction_group_for_sstable(const sstables::shared_sstable& sst) const { auto [first_id, first_range_side] = storage_group_of(sst->get_first_decorated_key().token()); auto [last_id, last_range_side] = storage_group_of(sst->get_last_decorated_key().token()); @@ -1079,7 +1079,7 @@ compaction_group& tablet_storage_group_manager::compaction_group_for_sstable(con return *sg.select_compaction_group(first_range_side); } -compaction_group& table::compaction_group_for_sstable(const sstables::shared_sstable& sst) const noexcept { +compaction_group& table::compaction_group_for_sstable(const sstables::shared_sstable& sst) const { return _sg_manager->compaction_group_for_sstable(sst); }