You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2020/02/21 05:06:31 UTC
[kudu] branch master updated: [master] call Shutdown() in destructor
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 16ad354 [master] call Shutdown() in destructor
16ad354 is described below
commit 16ad354482151e8cc27f4640250b0337e5219fd7
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Thu Feb 20 18:30:51 2020 -0800
[master] call Shutdown() in destructor
Master was reported crashing in tests with stack like below when
catalog manager failed to initialize:
F0220 11:54:37.240003 1700 master.cc:128] Check failed: kRunning != state_ (2 vs. 2)
*** Check failure stack trace: ***
@ 0x7ff77ba68202 google::LogMessage::Flush()
@ 0x7ff77ba6c93b google::LogMessageFatal::~LogMessageFatal()
@ 0x7ff7876ed9ac kudu::master::Master::~Master()
@ 0x7ff78770f797 kudu::master::RunMasterServer()
@ 0x4dd00f kudu::master::MasterMain()
I took a quick look into the code and if my understanding is correct,
nothing prevents us calling Shutdown() in destructor, like TabletServer
does.
This patch updates Master's destructor to call Shutdown() to avoid
unneeded crashes like above and unify the behavior of Master and
TabletServer objects.
I also did cosmetic updates to the code to squelch cland-tidy warnings.
Change-Id: Icc3c5d0b183175ec6a5f288a30fb8a24bd983962
Reviewed-on: http://gerrit.cloudera.org:8080/15261
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
src/kudu/kserver/kserver.h | 6 ++--
src/kudu/master/master.cc | 51 ++++++++++++++++-----------------
src/kudu/master/master.h | 17 +++++++----
src/kudu/server/server_base.cc | 60 +++++++++++++++++++--------------------
src/kudu/server/server_base.h | 9 +++++-
src/kudu/tserver/tablet_server.cc | 4 +--
src/kudu/tserver/tablet_server.h | 11 +++++--
7 files changed, 89 insertions(+), 69 deletions(-)
diff --git a/src/kudu/kserver/kserver.h b/src/kudu/kserver/kserver.h
index dc0b516..d63f212 100644
--- a/src/kudu/kserver/kserver.h
+++ b/src/kudu/kserver/kserver.h
@@ -51,13 +51,13 @@ class KuduServer : public server::ServerBase {
// Finalizes the initialization of a KuduServer by performing any member
// initializations that may fail.
- virtual Status Init() override;
+ Status Init() override;
// Exposes an initialized KuduServer to the network or filesystem.
- virtual Status Start() override;
+ Status Start() override;
// Shuts down a KuduServer instance.
- virtual void Shutdown() override;
+ void Shutdown() override;
ThreadPool* tablet_prepare_pool() const { return tablet_prepare_pool_.get(); }
ThreadPool* tablet_apply_pool() const { return tablet_apply_pool_.get(); }
diff --git a/src/kudu/master/master.cc b/src/kudu/master/master.cc
index 650b4fc..23e2104 100644
--- a/src/kudu/master/master.cc
+++ b/src/kudu/master/master.cc
@@ -125,7 +125,7 @@ Master::Master(const MasterOptions& opts)
}
Master::~Master() {
- CHECK_NE(kRunning, state_);
+ ShutdownImpl();
}
Status Master::Init() {
@@ -169,10 +169,9 @@ Status Master::Start() {
Status Master::StartAsync() {
CHECK_EQ(kInitialized, state_);
fs_manager_->SetErrorNotificationCb(ErrorHandlerType::DISK_ERROR,
- Bind(&Master::CrashMasterOnDiskError, Unretained(this)));
+ Bind(&Master::CrashMasterOnDiskError));
fs_manager_->SetErrorNotificationCb(ErrorHandlerType::CFILE_CORRUPTION,
- Bind(&Master::CrashMasterOnCFileCorruption,
- Unretained(this)));
+ Bind(&Master::CrashMasterOnCFileCorruption));
RETURN_NOT_OK(maintenance_manager_->Start());
@@ -240,28 +239,6 @@ Status Master::WaitUntilCatalogManagerIsLeaderAndReadyForTests(const MonoDelta&
s.ToString());
}
-void Master::Shutdown() {
- if (state_ == kRunning) {
- const string name = rpc_server_->ToString();
- LOG(INFO) << "Master@" << name << " shutting down...";
-
- // 1. Stop accepting new RPCs.
- UnregisterAllServices();
-
- // 2. Shut down the master's subsystems.
- init_pool_->Shutdown();
- maintenance_manager_->Shutdown();
- catalog_manager_->Shutdown();
- fs_manager_->UnsetErrorNotificationCb(ErrorHandlerType::DISK_ERROR);
- fs_manager_->UnsetErrorNotificationCb(ErrorHandlerType::CFILE_CORRUPTION);
-
- // 3. Shut down generic subsystems.
- KuduServer::Shutdown();
- LOG(INFO) << "Master@" << name << " shutdown complete.";
- }
- state_ = kStopped;
-}
-
Status Master::GetMasterRegistration(ServerRegistrationPB* reg) const {
if (!registration_initialized_.load(std::memory_order_acquire)) {
return Status::ServiceUnavailable("Master startup not complete");
@@ -294,6 +271,28 @@ Status Master::InitMasterRegistration() {
return Status::OK();
}
+void Master::ShutdownImpl() {
+ if (kInitialized == state_ || kRunning == state_) {
+ const string name = rpc_server_->ToString();
+ LOG(INFO) << "Master@" << name << " shutting down...";
+
+ // 1. Stop accepting new RPCs.
+ UnregisterAllServices();
+
+ // 2. Shut down the master's subsystems.
+ init_pool_->Shutdown();
+ maintenance_manager_->Shutdown();
+ catalog_manager_->Shutdown();
+ fs_manager_->UnsetErrorNotificationCb(ErrorHandlerType::DISK_ERROR);
+ fs_manager_->UnsetErrorNotificationCb(ErrorHandlerType::CFILE_CORRUPTION);
+
+ // 3. Shut down generic subsystems.
+ KuduServer::Shutdown();
+ LOG(INFO) << "Master@" << name << " shutdown complete.";
+ }
+ state_ = kStopped;
+}
+
void Master::CrashMasterOnDiskError(const string& uuid) {
LOG(FATAL) << Substitute("Disk error detected on data directory $0", uuid);
}
diff --git a/src/kudu/master/master.h b/src/kudu/master/master.h
index 4f7ce66..3f77074 100644
--- a/src/kudu/master/master.h
+++ b/src/kudu/master/master.h
@@ -60,9 +60,11 @@ class Master : public kserver::KuduServer {
explicit Master(const MasterOptions& opts);
~Master();
- virtual Status Init() override;
- virtual Status Start() override;
- virtual void Shutdown() override;
+ Status Init() override;
+ Status Start() override;
+ void Shutdown() override {
+ ShutdownImpl();
+ }
Status StartAsync();
Status WaitForCatalogManagerInit() const;
@@ -106,10 +108,10 @@ class Master : public kserver::KuduServer {
Status GetMasterHostPorts(std::vector<HostPortPB>* hostports) const;
// Crash the master on disk error.
- void CrashMasterOnDiskError(const std::string& uuid);
+ static void CrashMasterOnDiskError(const std::string& uuid);
// Crash the master on CFile corruption.
- void CrashMasterOnCFileCorruption(const std::string& tablet_id);
+ static void CrashMasterOnCFileCorruption(const std::string& tablet_id);
bool IsShutdown() const {
return state_ == kStopped;
@@ -129,6 +131,11 @@ class Master : public kserver::KuduServer {
// Requires that the web server and RPC server have been started.
Status InitMasterRegistration();
+ // A method for internal use in the destructor. Some static code analyzers
+ // issue a warning if calling a virtual function from destructor even if it's
+ // safe in a particular case.
+ void ShutdownImpl();
+
enum MasterState {
kStopped,
kInitialized,
diff --git a/src/kudu/server/server_base.cc b/src/kudu/server/server_base.cc
index 36b5093..b666d6f 100644
--- a/src/kudu/server/server_base.cc
+++ b/src/kudu/server/server_base.cc
@@ -449,7 +449,7 @@ ServerBase::ServerBase(string name, const ServerBaseOptions& options,
}
ServerBase::~ServerBase() {
- Shutdown();
+ ShutdownImpl();
}
Sockaddr ServerBase::first_rpc_address() const {
@@ -723,6 +723,35 @@ void ServerBase::ExcessLogFileDeleterThread() {
}
}
+void ServerBase::ShutdownImpl() {
+ // First, stop accepting incoming requests and wait for any outstanding
+ // requests to finish processing.
+ //
+ // Note: prior to Messenger::Shutdown, it is assumed that any incoming RPCs
+ // deferred from reactor threads have already been cleaned up.
+ if (web_server_) {
+ web_server_->Stop();
+ }
+ rpc_server_->Shutdown();
+ if (messenger_) {
+ messenger_->Shutdown();
+ }
+
+ // Next, shut down remaining server components.
+ stop_background_threads_latch_.CountDown();
+ if (diag_log_) {
+ diag_log_->Stop();
+ }
+ if (excess_log_deleter_thread_) {
+ excess_log_deleter_thread_->Join();
+ }
+#ifdef TCMALLOC_ENABLED
+ if (tcmalloc_memory_gc_thread_) {
+ tcmalloc_memory_gc_thread_->Join();
+ }
+#endif
+}
+
#ifdef TCMALLOC_ENABLED
Status ServerBase::StartTcmallocMemoryGcThread() {
return Thread::Create("server", "tcmalloc-memory-gc", &ServerBase::TcmallocMemoryGcThread,
@@ -774,35 +803,6 @@ Status ServerBase::Start() {
return Status::OK();
}
-void ServerBase::Shutdown() {
- // First, stop accepting incoming requests and wait for any outstanding
- // requests to finish processing.
- //
- // Note: prior to Messenger::Shutdown, it is assumed that any incoming RPCs
- // deferred from reactor threads have already been cleaned up.
- if (web_server_) {
- web_server_->Stop();
- }
- rpc_server_->Shutdown();
- if (messenger_) {
- messenger_->Shutdown();
- }
-
- // Next, shut down remaining server components.
- stop_background_threads_latch_.CountDown();
- if (diag_log_) {
- diag_log_->Stop();
- }
- if (excess_log_deleter_thread_) {
- excess_log_deleter_thread_->Join();
- }
-#ifdef TCMALLOC_ENABLED
- if (tcmalloc_memory_gc_thread_) {
- tcmalloc_memory_gc_thread_->Join();
- }
-#endif
-}
-
void ServerBase::UnregisterAllServices() {
messenger_->UnregisterAllServices();
}
diff --git a/src/kudu/server/server_base.h b/src/kudu/server/server_base.h
index c744007..93aaf16 100644
--- a/src/kudu/server/server_base.h
+++ b/src/kudu/server/server_base.h
@@ -141,7 +141,9 @@ class ServerBase {
virtual Status Start();
// Shuts down the server.
- virtual void Shutdown();
+ virtual void Shutdown() {
+ ShutdownImpl();
+ }
// Registers a new RPC service. Once Start() is called, the server will
// process and dispatch incoming RPCs belonging to this service.
@@ -217,6 +219,11 @@ class ServerBase {
Status StartExcessLogFileDeleterThread();
void ExcessLogFileDeleterThread();
+ // A method for internal use in the destructor. Some static code analyzers
+ // issue a warning if calling a virtual function from destructor even if it's
+ // safe in a particular case.
+ void ShutdownImpl();
+
#ifdef TCMALLOC_ENABLED
// Start thread to GC tcmalloc allocated memory.
Status StartTcmallocMemoryGcThread();
diff --git a/src/kudu/tserver/tablet_server.cc b/src/kudu/tserver/tablet_server.cc
index a120e21..b82d359 100644
--- a/src/kudu/tserver/tablet_server.cc
+++ b/src/kudu/tserver/tablet_server.cc
@@ -64,7 +64,7 @@ TabletServer::TabletServer(const TabletServerOptions& opts)
}
TabletServer::~TabletServer() {
- Shutdown();
+ ShutdownImpl();
}
Status TabletServer::Init() {
@@ -148,7 +148,7 @@ Status TabletServer::Start() {
return Status::OK();
}
-void TabletServer::Shutdown() {
+void TabletServer::ShutdownImpl() {
if (kInitialized == state_ || kRunning == state_) {
const string name = rpc_server_->ToString();
LOG(INFO) << "TabletServer@" << name << " shutting down...";
diff --git a/src/kudu/tserver/tablet_server.h b/src/kudu/tserver/tablet_server.h
index bce7773..2382f92 100644
--- a/src/kudu/tserver/tablet_server.h
+++ b/src/kudu/tserver/tablet_server.h
@@ -55,13 +55,15 @@ class TabletServer : public kserver::KuduServer {
// Some initialization tasks are asynchronous, such as the bootstrapping
// of tablets. Caller can block, waiting for the initialization to fully
// complete by calling WaitInited().
- virtual Status Init() override;
+ Status Init() override;
// Waits for the tablet server to complete the initialization.
Status WaitInited();
Status Start() override;
- void Shutdown() override;
+ void Shutdown() override {
+ ShutdownImpl();
+ }
std::string ToString() const;
@@ -100,6 +102,11 @@ class TabletServer : public kserver::KuduServer {
kRunning
};
+ // A method for internal use in the destructor. Some static code analyzers
+ // issue a warning if calling a virtual function from destructor even if it's
+ // safe in a particular case.
+ void ShutdownImpl();
+
TabletServerState state_;
std::atomic<bool> quiescing_;