From 310ce4276f709cc4237503648fb6e62f9003fb7b Mon Sep 17 00:00:00 2001 From: zhangyifan27 Date: Tue, 26 Jan 2021 11:40:36 +0800 Subject: [PATCH 1/5] refactor: use shared_ptr to replace raw pointer --- .../dist/failure_detector/failure_detector.h | 4 +-- .../dsn/dist/failure_detector_multimaster.h | 2 +- src/failure_detector/failure_detector.cpp | 33 ++++++++----------- .../failure_detector_multimaster.cpp | 5 --- src/replica/mutation_log.h | 2 +- src/replica/replica_stub.cpp | 15 ++------- src/replica/replica_stub.h | 2 +- 7 files changed, 20 insertions(+), 43 deletions(-) diff --git a/include/dsn/dist/failure_detector/failure_detector.h b/include/dsn/dist/failure_detector/failure_detector.h index 4312093100..3e83a664fa 100644 --- a/include/dsn/dist/failure_detector/failure_detector.h +++ b/include/dsn/dist/failure_detector/failure_detector.h @@ -93,7 +93,7 @@ class failure_detector : public failure_detector_service, { public: failure_detector(); - virtual ~failure_detector() { unregister_ctrl_commands(); } + virtual ~failure_detector(); virtual void on_ping(const beacon_msg &beacon, ::dsn::rpc_replier &reply); @@ -109,7 +109,7 @@ class failure_detector : public failure_detector_service, uint32_t grace_seconds, bool use_allow_list = false); - error_code stop(); + void stop(); uint32_t get_lease_ms() const { return _lease_milliseconds; } uint32_t get_grace_ms() const { return _grace_milliseconds; } diff --git a/include/dsn/dist/failure_detector_multimaster.h b/include/dsn/dist/failure_detector_multimaster.h index 9aedada62f..7b7351b21b 100644 --- a/include/dsn/dist/failure_detector_multimaster.h +++ b/include/dsn/dist/failure_detector_multimaster.h @@ -49,7 +49,7 @@ class slave_failure_detector_with_multimaster : public dsn::fd::failure_detector slave_failure_detector_with_multimaster(std::vector<::dsn::rpc_address> &meta_servers, std::function &&master_disconnected_callback, std::function &&master_connected_callback); - ~slave_failure_detector_with_multimaster(void); + virtual ~slave_failure_detector_with_multimaster() {} virtual void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context); diff --git a/src/failure_detector/failure_detector.cpp b/src/failure_detector/failure_detector.cpp index 3353239981..ddddae5f7e 100644 --- a/src/failure_detector/failure_detector.cpp +++ b/src/failure_detector/failure_detector.cpp @@ -56,6 +56,12 @@ failure_detector::failure_detector() _is_started = false; } +failure_detector::~failure_detector() +{ + stop(); + unregister_ctrl_commands(); +} + void failure_detector::register_ctrl_commands() { static std::once_flag flag; @@ -100,30 +106,17 @@ error_code failure_detector::start(uint32_t check_interval_seconds, return ERR_OK; } -error_code failure_detector::stop() +void failure_detector::stop() { + _tracker.cancel_outstanding_tasks(); + + zauto_lock l(_lock); if (_is_started == false) { - return ERR_OK; + return; } - _is_started = false; - - { - zauto_lock l(_lock); - for (auto &m : _masters) { - m.second.send_beacon_timer->cancel(true); - } - - _masters.clear(); - _workers.clear(); - } - - if (_check_task != nullptr) { - _check_task->cancel(true); - _check_task = nullptr; - } - - return ERR_OK; + _masters.clear(); + _workers.clear(); } void failure_detector::register_master(::dsn::rpc_address target) diff --git a/src/failure_detector/failure_detector_multimaster.cpp b/src/failure_detector/failure_detector_multimaster.cpp index a4593623c0..5c04a91c28 100644 --- a/src/failure_detector/failure_detector_multimaster.cpp +++ b/src/failure_detector/failure_detector_multimaster.cpp @@ -63,11 +63,6 @@ slave_failure_detector_with_multimaster::slave_failure_detector_with_multimaster _master_connected_callback = std::move(master_connected_callback); } -slave_failure_detector_with_multimaster::~slave_failure_detector_with_multimaster(void) -{ - _tracker.cancel_outstanding_tasks(); -} - void slave_failure_detector_with_multimaster::set_leader_for_test(rpc_address meta) { _meta_servers.group_address()->set_leader(meta); diff --git a/src/replica/mutation_log.h b/src/replica/mutation_log.h index e1efbb2b15..cd077d57fa 100644 --- a/src/replica/mutation_log.h +++ b/src/replica/mutation_log.h @@ -99,7 +99,7 @@ class mutation_log : public ref_counter // mutation_log(const std::string &dir, int32_t max_log_file_mb, gpid gpid, replica *r = nullptr); - virtual ~mutation_log() = default; + virtual ~mutation_log() { close(); } // // initialization diff --git a/src/replica/replica_stub.cpp b/src/replica/replica_stub.cpp index b00b049935..d8c50d0dcc 100644 --- a/src/replica/replica_stub.cpp +++ b/src/replica/replica_stub.cpp @@ -766,10 +766,10 @@ void replica_stub::initialize_start() // init liveness monitor dassert(NS_Disconnected == _state, ""); if (_options.fd_disabled == false) { - _failure_detector = new ::dsn::dist::slave_failure_detector_with_multimaster( + _failure_detector.reset(new dsn::dist::slave_failure_detector_with_multimaster( _options.meta_servers, [this]() { this->on_meta_server_disconnected(); }, - [this]() { this->on_meta_server_connected(); }); + [this]() { this->on_meta_server_connected(); })); auto err = _failure_detector->start(_options.fd_check_interval_seconds, _options.fd_beacon_interval_seconds, @@ -2511,17 +2511,6 @@ void replica_stub::close() _replicas.erase(_replicas.begin()); } } - - if (_failure_detector != nullptr) { - _failure_detector->stop(); - delete _failure_detector; - _failure_detector = nullptr; - } - - if (_log != nullptr) { - _log->close(); - _log = nullptr; - } } std::string replica_stub::get_replica_dir(const char *app_type, gpid id, bool create_new) diff --git a/src/replica/replica_stub.h b/src/replica/replica_stub.h index 1e6ac91644..43a76b32f2 100644 --- a/src/replica/replica_stub.h +++ b/src/replica/replica_stub.h @@ -334,7 +334,7 @@ class replica_stub : public serverlet, public ref_counter ::dsn::rpc_address _primary_address; char _primary_address_str[64]; - ::dsn::dist::slave_failure_detector_with_multimaster *_failure_detector; + std::shared_ptr _failure_detector; mutable zlock _state_lock; volatile replica_node_state _state; From 810c37cc8c02695ed65cdc595004f348b15eb255 Mon Sep 17 00:00:00 2001 From: Yingchun Lai <405403881@qq.com> Date: Sun, 7 Feb 2021 11:04:19 +0800 Subject: [PATCH 2/5] Update src/failure_detector/failure_detector.cpp --- src/failure_detector/failure_detector.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/failure_detector/failure_detector.cpp b/src/failure_detector/failure_detector.cpp index ddddae5f7e..3b26d1b620 100644 --- a/src/failure_detector/failure_detector.cpp +++ b/src/failure_detector/failure_detector.cpp @@ -111,7 +111,7 @@ void failure_detector::stop() _tracker.cancel_outstanding_tasks(); zauto_lock l(_lock); - if (_is_started == false) { + if (!_is_started) { return; } _is_started = false; From 00fb1f56a988962d0e8122dc55264777dcfd08db Mon Sep 17 00:00:00 2001 From: zhangyifan27 Date: Thu, 18 Feb 2021 10:40:03 +0800 Subject: [PATCH 3/5] fix --- include/dsn/dist/failure_detector_multimaster.h | 12 ++++++------ src/replica/mutation_log.h | 2 +- src/replica/replica_stub.cpp | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/include/dsn/dist/failure_detector_multimaster.h b/include/dsn/dist/failure_detector_multimaster.h index 7b7351b21b..1259eac541 100644 --- a/include/dsn/dist/failure_detector_multimaster.h +++ b/include/dsn/dist/failure_detector_multimaster.h @@ -49,20 +49,20 @@ class slave_failure_detector_with_multimaster : public dsn::fd::failure_detector slave_failure_detector_with_multimaster(std::vector<::dsn::rpc_address> &meta_servers, std::function &&master_disconnected_callback, std::function &&master_connected_callback); - virtual ~slave_failure_detector_with_multimaster() {} + virtual ~slave_failure_detector_with_multimaster() override {} - virtual void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context); + virtual void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context) override; // client side - virtual void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes); - virtual void on_master_connected(::dsn::rpc_address node); + virtual void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes) override; + virtual void on_master_connected(::dsn::rpc_address node) override; // server side - virtual void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes) + virtual void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes) override { dassert(false, "invalid execution flow"); } - virtual void on_worker_connected(::dsn::rpc_address node) + virtual void on_worker_connected(::dsn::rpc_address node) override { dassert(false, "invalid execution flow"); } diff --git a/src/replica/mutation_log.h b/src/replica/mutation_log.h index cd077d57fa..e1efbb2b15 100644 --- a/src/replica/mutation_log.h +++ b/src/replica/mutation_log.h @@ -99,7 +99,7 @@ class mutation_log : public ref_counter // mutation_log(const std::string &dir, int32_t max_log_file_mb, gpid gpid, replica *r = nullptr); - virtual ~mutation_log() { close(); } + virtual ~mutation_log() = default; // // initialization diff --git a/src/replica/replica_stub.cpp b/src/replica/replica_stub.cpp index d8c50d0dcc..3f2966aa3c 100644 --- a/src/replica/replica_stub.cpp +++ b/src/replica/replica_stub.cpp @@ -766,10 +766,10 @@ void replica_stub::initialize_start() // init liveness monitor dassert(NS_Disconnected == _state, ""); if (_options.fd_disabled == false) { - _failure_detector.reset(new dsn::dist::slave_failure_detector_with_multimaster( + _failure_detector = std::make_shared( _options.meta_servers, [this]() { this->on_meta_server_disconnected(); }, - [this]() { this->on_meta_server_connected(); })); + [this]() { this->on_meta_server_connected(); }); auto err = _failure_detector->start(_options.fd_check_interval_seconds, _options.fd_beacon_interval_seconds, From 28312e2146069a2217950f69aaa55ba143730f51 Mon Sep 17 00:00:00 2001 From: zhangyifan27 Date: Thu, 18 Feb 2021 18:32:07 +0800 Subject: [PATCH 4/5] fix --- include/dsn/dist/failure_detector_multimaster.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/dsn/dist/failure_detector_multimaster.h b/include/dsn/dist/failure_detector_multimaster.h index 1259eac541..7b7351b21b 100644 --- a/include/dsn/dist/failure_detector_multimaster.h +++ b/include/dsn/dist/failure_detector_multimaster.h @@ -49,20 +49,20 @@ class slave_failure_detector_with_multimaster : public dsn::fd::failure_detector slave_failure_detector_with_multimaster(std::vector<::dsn::rpc_address> &meta_servers, std::function &&master_disconnected_callback, std::function &&master_connected_callback); - virtual ~slave_failure_detector_with_multimaster() override {} + virtual ~slave_failure_detector_with_multimaster() {} - virtual void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context) override; + virtual void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context); // client side - virtual void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes) override; - virtual void on_master_connected(::dsn::rpc_address node) override; + virtual void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes); + virtual void on_master_connected(::dsn::rpc_address node); // server side - virtual void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes) override + virtual void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes) { dassert(false, "invalid execution flow"); } - virtual void on_worker_connected(::dsn::rpc_address node) override + virtual void on_worker_connected(::dsn::rpc_address node) { dassert(false, "invalid execution flow"); } From 94e7f2b7a5d3dce0778c48d0a599209ed89e0dc9 Mon Sep 17 00:00:00 2001 From: zhangyifan27 Date: Sat, 20 Feb 2021 12:21:01 +0800 Subject: [PATCH 5/5] fix virtual/override --- include/dsn/dist/failure_detector_multimaster.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/dsn/dist/failure_detector_multimaster.h b/include/dsn/dist/failure_detector_multimaster.h index 7b7351b21b..2859396c0a 100644 --- a/include/dsn/dist/failure_detector_multimaster.h +++ b/include/dsn/dist/failure_detector_multimaster.h @@ -51,18 +51,18 @@ class slave_failure_detector_with_multimaster : public dsn::fd::failure_detector std::function &&master_connected_callback); virtual ~slave_failure_detector_with_multimaster() {} - virtual void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context); + void end_ping(::dsn::error_code err, const fd::beacon_ack &ack, void *context) override; // client side - virtual void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes); - virtual void on_master_connected(::dsn::rpc_address node); + void on_master_disconnected(const std::vector<::dsn::rpc_address> &nodes) override; + void on_master_connected(::dsn::rpc_address node) override; // server side - virtual void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes) + void on_worker_disconnected(const std::vector<::dsn::rpc_address> &nodes) override { dassert(false, "invalid execution flow"); } - virtual void on_worker_connected(::dsn::rpc_address node) + void on_worker_connected(::dsn::rpc_address node) override { dassert(false, "invalid execution flow"); }