From dd7c7c301dfe1e811a61b5d7848362c89ff7279c Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 Jul 2024 15:47:46 +0300 Subject: [PATCH 1/3] hints: Const-ify gossiper references and anchor pointers There are two places in hints code that need gossiper: hist_sender calling gossiper::is_alive() and endpoint_downtime_not_bigger_than() helper in manager. Both can live with const gossiper, so the dependency references and anchor pointers can be restricted to const too. Signed-off-by: Pavel Emelyanov --- db/hints/internal/hint_sender.cc | 2 +- db/hints/internal/hint_sender.hh | 4 ++-- db/hints/manager.cc | 2 +- db/hints/manager.hh | 6 +++--- db/hints/resource_manager.cc | 2 +- db/hints/resource_manager.hh | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/db/hints/internal/hint_sender.cc b/db/hints/internal/hint_sender.cc index 16ca83ad4d..bac4a17fe4 100644 --- a/db/hints/internal/hint_sender.cc +++ b/db/hints/internal/hint_sender.cc @@ -158,7 +158,7 @@ const column_mapping& hint_sender::get_column_mapping(lw_shared_ptrsecond; } -hint_sender::hint_sender(hint_endpoint_manager& parent, service::storage_proxy& local_storage_proxy,replica::database& local_db, gms::gossiper& local_gossiper) noexcept +hint_sender::hint_sender(hint_endpoint_manager& parent, service::storage_proxy& local_storage_proxy,replica::database& local_db, const gms::gossiper& local_gossiper) noexcept : _stopped(make_ready_future<>()) , _ep_key(parent.end_point_key()) , _ep_manager(parent) diff --git a/db/hints/internal/hint_sender.hh b/db/hints/internal/hint_sender.hh index 0d990cf2d4..746c66a38a 100644 --- a/db/hints/internal/hint_sender.hh +++ b/db/hints/internal/hint_sender.hh @@ -112,13 +112,13 @@ private: service::storage_proxy& _proxy; replica::database& _db; seastar::scheduling_group _hints_cpu_sched_group; - gms::gossiper& _gossiper; + const gms::gossiper& _gossiper; seastar::shared_mutex& _file_update_mutex; std::multimap>>> _replay_waiters; public: - hint_sender(hint_endpoint_manager& parent, service::storage_proxy& local_storage_proxy, replica::database& local_db, gms::gossiper& local_gossiper) noexcept; + hint_sender(hint_endpoint_manager& parent, service::storage_proxy& local_storage_proxy, replica::database& local_db, const gms::gossiper& local_gossiper) noexcept; ~hint_sender(); /// \brief A constructor that should be called from the copy/move-constructor of hint_endpoint_manager. diff --git a/db/hints/manager.cc b/db/hints/manager.cc index d7002bfb42..40132632da 100644 --- a/db/hints/manager.cc +++ b/db/hints/manager.cc @@ -196,7 +196,7 @@ void manager::register_metrics(const sstring& group_name) { }); } -future<> manager::start(shared_ptr gossiper_ptr) { +future<> manager::start(shared_ptr gossiper_ptr) { _gossiper_anchor = std::move(gossiper_ptr); if (_proxy.features().host_id_based_hinted_handoff) { diff --git a/db/hints/manager.hh b/db/hints/manager.hh index eed7d57f84..c589f68046 100644 --- a/db/hints/manager.hh +++ b/db/hints/manager.hh @@ -115,7 +115,7 @@ private: node_to_hint_store_factory_type _store_factory; host_filter _host_filter; service::storage_proxy& _proxy; - shared_ptr _gossiper_anchor; + shared_ptr _gossiper_anchor; int64_t _max_hint_window_us = 0; replica::database& _local_db; @@ -172,7 +172,7 @@ public: public: void register_metrics(const sstring& group_name); - future<> start(shared_ptr gossiper_ptr); + future<> start(shared_ptr gossiper_ptr); future<> stop(); bool store_hint(endpoint_id host_id, gms::inet_address ip, schema_ptr s, lw_shared_ptr fm, tracing::trace_state_ptr tr_state) noexcept; @@ -294,7 +294,7 @@ private: return _proxy; } - gms::gossiper& local_gossiper() const noexcept { + const gms::gossiper& local_gossiper() const noexcept { return *_gossiper_anchor; } diff --git a/db/hints/resource_manager.cc b/db/hints/resource_manager.cc index ceae530c71..5df03469e4 100644 --- a/db/hints/resource_manager.cc +++ b/db/hints/resource_manager.cc @@ -202,7 +202,7 @@ void space_watchdog::on_timer() { } } -future<> resource_manager::start(shared_ptr gossiper_ptr) { +future<> resource_manager::start(shared_ptr gossiper_ptr) { _gossiper_ptr = std::move(gossiper_ptr); return with_semaphore(_operation_lock, 1, [this] () { diff --git a/db/hints/resource_manager.hh b/db/hints/resource_manager.hh index ec154fec36..7ad2d53bbd 100644 --- a/db/hints/resource_manager.hh +++ b/db/hints/resource_manager.hh @@ -130,7 +130,7 @@ class resource_manager { space_watchdog _space_watchdog; service::storage_proxy& _proxy; - shared_ptr _gossiper_ptr; + shared_ptr _gossiper_ptr; enum class state { running, @@ -186,7 +186,7 @@ public: future> get_send_units_for(size_t buf_size); size_t sending_queue_length() const; - future<> start(shared_ptr gossiper_ptr); + future<> start(shared_ptr gossiper_ptr); future<> stop() noexcept; /// \brief Allows replaying hints for managers which are registered now or will be in the future. From a1dbaba9e1f0c83bc7dd0221c612c869af489be7 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 Jul 2024 15:49:35 +0300 Subject: [PATCH 2/3] proxy: Use remote gossiper to start hints resource manager By the time hinst resource manager is started, proxy already has its remote part initialized. Remote returns const gossiper pointer, but after previous change hints code can live with it. Signed-off-by: Pavel Emelyanov --- service/storage_proxy.cc | 4 ++-- service/storage_proxy.hh | 2 +- service/storage_service.cc | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/service/storage_proxy.cc b/service/storage_proxy.cc index d86dfe2441..cb28db9912 100644 --- a/service/storage_proxy.cc +++ b/service/storage_proxy.cc @@ -6542,12 +6542,12 @@ storage_proxy::query_nonsingular_data_locally(schema_ptr s, lw_shared_ptr storage_proxy::start_hints_manager(shared_ptr g) { +future<> storage_proxy::start_hints_manager() { if (!_hints_manager.is_disabled_for_all()) { co_await _hints_resource_manager.register_manager(_hints_manager); } co_await _hints_resource_manager.register_manager(_hints_for_views_manager); - co_await _hints_resource_manager.start(std::move(g)); + co_await _hints_resource_manager.start(remote().gossiper().shared_from_this()); } void storage_proxy::allow_replaying_hints() noexcept { diff --git a/service/storage_proxy.hh b/service/storage_proxy.hh index 9dca5d99c3..3e03cd8c10 100644 --- a/service/storage_proxy.hh +++ b/service/storage_proxy.hh @@ -694,7 +694,7 @@ public: mutation get_batchlog_mutation_for(const std::vector& mutations, const utils::UUID& id, int32_t version, db_clock::time_point now); future<> stop(); - future<> start_hints_manager(shared_ptr); + future<> start_hints_manager(); void allow_replaying_hints() noexcept; future<> abort_view_writes(); diff --git a/service/storage_service.cc b/service/storage_service.cc index fa71e227f4..38761af893 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -1783,8 +1783,8 @@ future<> storage_service::join_token_ring(sharded Date: Fri, 26 Jul 2024 15:52:15 +0300 Subject: [PATCH 3/3] storage_service: Remote gossiper argument from join_cluster() This pointer was only needed to pull all the way down the hints resource manager start() method. It's no longer needed for that. Signed-off-by: Pavel Emelyanov --- main.cc | 2 +- service/storage_service.cc | 5 ++--- service/storage_service.hh | 3 +-- test/lib/cql_test_env.cc | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/main.cc b/main.cc index 75ea339aa2..83750cc038 100644 --- a/main.cc +++ b/main.cc @@ -1914,7 +1914,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl }).get(); with_scheduling_group(maintenance_scheduling_group, [&] { - return ss.local().join_cluster(sys_dist_ks, proxy, gossiper, service::start_hint_manager::yes, generation_number); + return ss.local().join_cluster(sys_dist_ks, proxy, service::start_hint_manager::yes, generation_number); }).get(); supervisor::notify("starting tracing"); diff --git a/service/storage_service.cc b/service/storage_service.cc index 38761af893..81841b3541 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -1485,7 +1485,6 @@ future<> storage_service::await_tablets_rebuilt(raft::server_id replaced_id) { future<> storage_service::join_token_ring(sharded& sys_dist_ks, sharded& proxy, - sharded& gossiper, std::unordered_set initial_contact_nodes, std::unordered_map loaded_endpoints, std::unordered_map loaded_peer_features, @@ -2874,7 +2873,7 @@ bool storage_service::is_topology_coordinator_enabled() const { } future<> storage_service::join_cluster(sharded& sys_dist_ks, sharded& proxy, - sharded& gossiper, start_hint_manager start_hm, gms::generation_type new_generation) { + start_hint_manager start_hm, gms::generation_type new_generation) { assert(this_shard_id() == 0); if (_sys_ks.local().was_decommissioned()) { @@ -2989,7 +2988,7 @@ future<> storage_service::join_cluster(sharded& } } - co_return co_await join_token_ring(sys_dist_ks, proxy, gossiper, std::move(initial_contact_nodes), + co_return co_await join_token_ring(sys_dist_ks, proxy, std::move(initial_contact_nodes), std::move(loaded_endpoints), std::move(loaded_peer_features), get_ring_delay(), start_hm, new_generation); } diff --git a/service/storage_service.hh b/service/storage_service.hh index 92fb654041..6471e8eac0 100644 --- a/service/storage_service.hh +++ b/service/storage_service.hh @@ -375,7 +375,7 @@ public: const std::unordered_map& loaded_peer_features); future<> join_cluster(sharded& sys_dist_ks, sharded& proxy, - sharded& gossiper_ptr, start_hint_manager start_hm, gms::generation_type new_generation); + start_hint_manager start_hm, gms::generation_type new_generation); void set_group0(service::raft_group0&); @@ -396,7 +396,6 @@ private: bool is_first_node(); future<> join_token_ring(sharded& sys_dist_ks, sharded& proxy, - sharded& gossiper, std::unordered_set initial_contact_nodes, std::unordered_map loaded_endpoints, std::unordered_map loaded_peer_features, diff --git a/test/lib/cql_test_env.cc b/test/lib/cql_test_env.cc index f0d45e95fb..dbbea7127c 100644 --- a/test/lib/cql_test_env.cc +++ b/test/lib/cql_test_env.cc @@ -937,7 +937,7 @@ private: group0_service.setup_group0_if_exist(_sys_ks.local(), _ss.local(), _qp.local(), _mm.local()).get(); try { - _ss.local().join_cluster(_sys_dist_ks, _proxy, _gossiper, service::start_hint_manager::no, generation_number).get(); + _ss.local().join_cluster(_sys_dist_ks, _proxy, service::start_hint_manager::no, generation_number).get(); } catch (std::exception& e) { // if any of the defers crashes too, we'll never see // the error