From 684c4143df363147f0c6f515126bd6e5f31e2a29 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 13 Dec 2020 11:50:23 +0200 Subject: [PATCH 1/4] storage_service: get_changed_ranges_for_leaving: don't hold token_metadata& across yield When yielding in clone_only_token_map or clone_after_all_left the token_metadata got with get_token_metadata() may go away. Use get_token_metadata_ptr() instead to hold on to it. And with that, we don't need to clone_only_token_map. `metadata` is not modified by calculate_natural_endpoints, so we can just refer to the immutable copy retrieved with get_token_metadata_ptr. Fixes #7790 Signed-off-by: Benny Halevy --- service/storage_service.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/service/storage_service.cc b/service/storage_service.cc index b46707f1ac..335aa79324 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -2489,17 +2489,16 @@ std::unordered_multimap storage_service::get_cha std::unordered_map> current_replica_endpoints; // Find (for each range) all nodes that store replicas for these ranges as well - const auto& tm = get_token_metadata(); - auto metadata = tm.clone_only_token_map().get0(); + auto tmptr = get_token_metadata_ptr(); for (auto& r : ranges) { seastar::thread::maybe_yield(); auto& ks = _db.local().find_keyspace(keyspace_name); auto end_token = r.end() ? r.end()->value() : dht::maximum_token(); - auto eps = ks.get_replication_strategy().calculate_natural_endpoints(end_token, metadata, utils::can_yield::yes); + auto eps = ks.get_replication_strategy().calculate_natural_endpoints(end_token, *tmptr, utils::can_yield::yes); current_replica_endpoints.emplace(r, std::move(eps)); } - auto temp = tm.clone_after_all_left().get0(); + auto temp = tmptr->clone_after_all_left().get0(); // endpoint might or might not be 'leaving'. If it was not leaving (that is, removenode // command was used), it is still present in temp and must be removed. From 89ed0705e830d51942f5293492b423a0b3e2839e Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 13 Dec 2020 12:01:49 +0200 Subject: [PATCH 2/4] storage_service: get_changed_ranges_for_leaving: release token_metadata_ptr sooner No need to hold on to the shared token_metadata_ptr after we got clone_after_all_left(). Signed-off-by: Benny Halevy --- service/storage_service.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/service/storage_service.cc b/service/storage_service.cc index 335aa79324..b4134daf28 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -2500,6 +2500,11 @@ std::unordered_multimap storage_service::get_cha auto temp = tmptr->clone_after_all_left().get0(); + // release the token_metadata_ptr + // as it's not needed from this point on + // after we got clone_after_all_left() + tmptr = nullptr; + // endpoint might or might not be 'leaving'. If it was not leaving (that is, removenode // command was used), it is still present in temp and must be removed. if (temp.is_member(endpoint)) { From f13913d2510292d9448aefff2519c9df7bba202b Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 13 Dec 2020 11:53:27 +0200 Subject: [PATCH 3/4] storage_service: get_changed_ranges_for_leaving: no need to maybe_yield for each token_range Now that we pass can_yield::yes to calculate_natural_endpoints for each token_range. Signed-off-by: Benny Halevy --- service/storage_service.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/service/storage_service.cc b/service/storage_service.cc index b4134daf28..1aa0f48181 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -2491,7 +2491,6 @@ std::unordered_multimap storage_service::get_cha // Find (for each range) all nodes that store replicas for these ranges as well auto tmptr = get_token_metadata_ptr(); for (auto& r : ranges) { - seastar::thread::maybe_yield(); auto& ks = _db.local().find_keyspace(keyspace_name); auto end_token = r.end() ? r.end()->value() : dht::maximum_token(); auto eps = ks.get_replication_strategy().calculate_natural_endpoints(end_token, *tmptr, utils::can_yield::yes); @@ -2519,7 +2518,6 @@ std::unordered_multimap storage_service::get_cha // not in the currentReplicaEndpoints list, will be needing the // range. for (auto& r : ranges) { - seastar::thread::maybe_yield(); auto& ks = _db.local().find_keyspace(keyspace_name); auto end_token = r.end() ? r.end()->value() : dht::maximum_token(); auto new_replica_endpoints = ks.get_replication_strategy().calculate_natural_endpoints(end_token, temp, utils::can_yield::yes); From 1fbc831dae0eb1550b1fe5df7718b1d8bcb58edc Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 13 Dec 2020 12:12:15 +0200 Subject: [PATCH 4/4] storage_service: get_new_source_ranges: don't hold token_metadata& across yield point Provide the token_metadata& to get_new_source_ranges by the caller, who keeps it valid throughout the call. Note that there is no need to clone_only_token_map since the token_metadata_ptr is immutable and can be used just as well for calling strat.get_range_addresses. Signed-off-by: Benny Halevy --- service/storage_service.cc | 8 ++++---- service/storage_service.hh | 6 +++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/service/storage_service.cc b/service/storage_service.cc index 1aa0f48181..116ecfc810 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -2602,7 +2602,8 @@ future<> storage_service::restore_replica_count(inet_address endpoint, inet_addr }); } return seastar::async([this, endpoint, notify_endpoint] { - auto streamer = make_lw_shared(_db, get_token_metadata_ptr(), _abort_source, get_broadcast_address(), "Restore_replica_count", streaming::stream_reason::removenode); + auto tmptr = get_token_metadata_ptr(); + auto streamer = make_lw_shared(_db, tmptr, _abort_source, get_broadcast_address(), "Restore_replica_count", streaming::stream_reason::removenode); auto my_address = get_broadcast_address(); auto non_system_keyspaces = _db.local().get_non_system_keyspaces(); for (const auto& keyspace_name : non_system_keyspaces) { @@ -2613,7 +2614,7 @@ future<> storage_service::restore_replica_count(inet_address endpoint, inet_addr my_new_ranges.emplace_back(x.first); } } - std::unordered_multimap source_ranges = get_new_source_ranges(keyspace_name, my_new_ranges); + std::unordered_multimap source_ranges = get_new_source_ranges(keyspace_name, my_new_ranges, *tmptr); std::unordered_map ranges_per_endpoint; for (auto& x : source_ranges) { ranges_per_endpoint[x.first].emplace_back(x.second); @@ -2819,11 +2820,10 @@ storage_service::set_tables_autocompaction(const sstring &keyspace, std::vector< // Must be called from a seastar thread std::unordered_multimap -storage_service::get_new_source_ranges(const sstring& keyspace_name, const dht::token_range_vector& ranges) { +storage_service::get_new_source_ranges(const sstring& keyspace_name, const dht::token_range_vector& ranges, const token_metadata& tm) { auto my_address = get_broadcast_address(); auto& ks = _db.local().find_keyspace(keyspace_name); auto& strat = ks.get_replication_strategy(); - auto tm = get_token_metadata().clone_only_token_map().get0(); std::unordered_map> range_addresses = strat.get_range_addresses(tm, utils::can_yield::yes); std::unordered_multimap source_ranges; diff --git a/service/storage_service.hh b/service/storage_service.hh index 717b055a26..511caed2e3 100644 --- a/service/storage_service.hh +++ b/service/storage_service.hh @@ -672,9 +672,13 @@ private: * * @param keyspaceName the keyspace ranges belong to * @param ranges the ranges to find sources for + * @param tm the token metadata * @return multimap of addresses to ranges the address is responsible for + * + * @note The function must be called from a seastar thread. + * The caller is responsible for keeping @ref tm valid across the call. */ - std::unordered_multimap get_new_source_ranges(const sstring& keyspaceName, const dht::token_range_vector& ranges); + std::unordered_multimap get_new_source_ranges(const sstring& keyspaceName, const dht::token_range_vector& ranges, const token_metadata& tm); public: future<> confirm_replication(inet_address node);