tablet_peer: clean up TabletStatusListener interface This class duplicated a lot of TabletPeer info for no particular reason. Its main purpose was to provide a clean callback for reporting status back to the TabletPeer, but over time it grew various unnecessary warts.
Change-Id: I06233b88a83859020cee1f96cff205952e2aa2da Reviewed-on: http://gerrit.cloudera.org:8080/4193 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e1f54cbe Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e1f54cbe Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e1f54cbe Branch: refs/heads/master Commit: e1f54cbe7c3e72ea2711edf354af6b8481639a6f Parents: ade0192 Author: Todd Lipcon <[email protected]> Authored: Wed Aug 31 16:53:45 2016 -0700 Committer: Adar Dembo <[email protected]> Committed: Thu Sep 1 22:00:48 2016 +0000 ---------------------------------------------------------------------- src/kudu/integration-tests/tablet_copy-itest.cc | 3 +- src/kudu/master/sys_catalog.cc | 4 +- src/kudu/tablet/tablet_bootstrap-test.cc | 3 +- src/kudu/tablet/tablet_bootstrap.cc | 49 ++++++-------------- src/kudu/tablet/tablet_bootstrap.h | 33 +------------ src/kudu/tablet/tablet_peer.cc | 23 ++++++--- src/kudu/tablet/tablet_peer.h | 30 +++++++++--- src/kudu/tserver/tablet_copy_client-test.cc | 5 +- src/kudu/tserver/tablet_server-test.cc | 2 +- src/kudu/tserver/tablet_service.cc | 2 +- src/kudu/tserver/ts_tablet_manager.cc | 8 ++-- src/kudu/tserver/tserver-path-handlers.cc | 2 +- 12 files changed, 68 insertions(+), 96 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/integration-tests/tablet_copy-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/tablet_copy-itest.cc b/src/kudu/integration-tests/tablet_copy-itest.cc index fba0520..43932f7 100644 --- a/src/kudu/integration-tests/tablet_copy-itest.cc +++ b/src/kudu/integration-tests/tablet_copy-itest.cc @@ -300,8 +300,7 @@ TEST_F(TabletCopyITest, TestDeleteTabletDuringTabletCopy) { TABLET_DATA_TOMBSTONED, boost::none, timeout)); // Now finish copying! - tablet::TabletStatusListener listener(meta); - ASSERT_OK(tc_client.FetchAll(&listener)); + ASSERT_OK(tc_client.FetchAll(nullptr /* no listener */)); ASSERT_OK(tc_client.Finish()); // Run destructor, which closes the remote session. http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/master/sys_catalog.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc index 17450ac..8999b7a 100644 --- a/src/kudu/master/sys_catalog.cc +++ b/src/kudu/master/sys_catalog.cc @@ -35,6 +35,7 @@ #include "kudu/consensus/opid_util.h" #include "kudu/consensus/quorum_util.h" #include "kudu/fs/fs_manager.h" +#include "kudu/gutil/casts.h" #include "kudu/gutil/strings/join.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/master/catalog_manager.h" @@ -66,6 +67,7 @@ using kudu::log::LogAnchorRegistry; using kudu::tablet::LatchTransactionCompletionCallback; using kudu::tablet::Tablet; using kudu::tablet::TabletPeer; +using kudu::tablet::TabletStatusListener; using kudu::tserver::WriteRequestPB; using kudu::tserver::WriteResponsePB; using std::shared_ptr; @@ -288,7 +290,7 @@ Status SysCatalogTable::SetupTablet(const scoped_refptr<tablet::TabletMetadata>& master_->mem_tracker(), scoped_refptr<rpc::ResultTracker>(), metric_registry_, - tablet_peer_->status_listener(), + implicit_cast<TabletStatusListener*>(tablet_peer_.get()), &tablet, &log, tablet_peer_->log_anchor_registry(), http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tablet/tablet_bootstrap-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/tablet_bootstrap-test.cc b/src/kudu/tablet/tablet_bootstrap-test.cc index 5cb97b4..83d7869 100644 --- a/src/kudu/tablet/tablet_bootstrap-test.cc +++ b/src/kudu/tablet/tablet_bootstrap-test.cc @@ -102,7 +102,6 @@ class BootstrapTest : public LogTestBase { Status RunBootstrapOnTestTablet(const scoped_refptr<TabletMetadata>& meta, shared_ptr<Tablet>* tablet, ConsensusBootstrapInfo* boot_info) { - gscoped_ptr<TabletStatusListener> listener(new TabletStatusListener(meta)); scoped_refptr<LogAnchorRegistry> log_anchor_registry(new LogAnchorRegistry()); // Now attempt to recover the log RETURN_NOT_OK(BootstrapTablet( @@ -111,7 +110,7 @@ class BootstrapTest : public LogTestBase { shared_ptr<MemTracker>(), scoped_refptr<rpc::ResultTracker>(), NULL, - listener.get(), + nullptr, // no status listener tablet, &log_, log_anchor_registry, http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tablet/tablet_bootstrap.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/tablet_bootstrap.cc b/src/kudu/tablet/tablet_bootstrap.cc index 135f7cc..845f4af 100644 --- a/src/kudu/tablet/tablet_bootstrap.cc +++ b/src/kudu/tablet/tablet_bootstrap.cc @@ -308,6 +308,10 @@ class TabletBootstrap { // Return a log prefix string in the standard "T xxx P yyy" format. string LogPrefix() const; + // Report a status message in the WAL as well as update the tablet peer's + // status. + void StatusMessage(const string& status); + scoped_refptr<TabletMetadata> meta_; scoped_refptr<Clock> clock_; shared_ptr<MemTracker> mem_tracker_; @@ -372,35 +376,10 @@ class TabletBootstrap { DISALLOW_COPY_AND_ASSIGN(TabletBootstrap); }; -TabletStatusListener::TabletStatusListener(const scoped_refptr<TabletMetadata>& meta) - : meta_(meta), - last_status_("") { -} - -const string TabletStatusListener::tablet_id() const { - return meta_->tablet_id(); -} - -const string TabletStatusListener::table_name() const { - return meta_->table_name(); -} - -const Partition& TabletStatusListener::partition() const { - return meta_->partition(); -} - -const Schema& TabletStatusListener::schema() const { - return meta_->schema(); -} - -TabletStatusListener::~TabletStatusListener() { -} - -void TabletStatusListener::StatusMessage(const string& status) { - LOG(INFO) << "T " << tablet_id() << " P " << meta_->fs_manager()->uuid() << ": " +void TabletBootstrap::StatusMessage(const string& status) { + LOG(INFO) << "T " << meta_->tablet_id() << " P " << meta_->fs_manager()->uuid() << ": " << status; - std::lock_guard<RWMutex> l(lock_); - last_status_ = status; + if (listener_) listener_->StatusMessage(status); } Status BootstrapTablet(const scoped_refptr<TabletMetadata>& meta, @@ -479,7 +458,7 @@ Status TabletBootstrap::Bootstrap(shared_ptr<Tablet>* rebuilt_tablet, meta_->PinFlush(); - listener_->StatusMessage("Bootstrap starting."); + StatusMessage("Bootstrap starting."); if (VLOG_IS_ON(1)) { TabletSuperBlockPB super_block; @@ -547,7 +526,7 @@ Status TabletBootstrap::FinishBootstrap(const string& message, log_)))); tablet_->MarkFinishedBootstrapping(); RETURN_NOT_OK(tablet_->metadata()->UnPinFlush()); - listener_->StatusMessage(message); + StatusMessage(message); rebuilt_tablet->reset(tablet_.release()); rebuilt_log->swap(log_); return Status::OK(); @@ -1104,11 +1083,11 @@ Status TabletBootstrap::PlaySegments(ConsensusBootstrapInfo* consensus_info) { // TODO: could be more granular here and log during the segments as well, // plus give info about number of MB processed, but this is better than // nothing. - listener_->StatusMessage(Substitute("Bootstrap replayed $0/$1 log segments. " - "Stats: $2. Pending: $3 replicates", - segment_count + 1, log_reader_->num_segments(), - stats_.ToString(), - state.pending_replicates.size())); + StatusMessage(Substitute("Bootstrap replayed $0/$1 log segments. " + "Stats: $2. Pending: $3 replicates", + segment_count + 1, log_reader_->num_segments(), + stats_.ToString(), + state.pending_replicates.size())); segment_count++; } http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tablet/tablet_bootstrap.h ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/tablet_bootstrap.h b/src/kudu/tablet/tablet_bootstrap.h index a7cdb6e..5dc72a3 100644 --- a/src/kudu/tablet/tablet_bootstrap.h +++ b/src/kudu/tablet/tablet_bootstrap.h @@ -54,38 +54,7 @@ class Clock; namespace tablet { class Tablet; class TabletMetadata; - -// A listener for logging the tablet related statuses as well as -// piping it into the web UI. -class TabletStatusListener { - public: - explicit TabletStatusListener(const scoped_refptr<TabletMetadata>& meta); - - ~TabletStatusListener(); - - void StatusMessage(const std::string& status); - - const std::string tablet_id() const; - - const std::string table_name() const; - - const Partition& partition() const; - - const Schema& schema() const; - - std::string last_status() const { - shared_lock<RWMutex> l(lock_); - return last_status_; - } - - private: - mutable RWMutex lock_; - - scoped_refptr<TabletMetadata> meta_; - std::string last_status_; - - DISALLOW_COPY_AND_ASSIGN(TabletStatusListener); -}; +class TabletStatusListener; extern const char* kLogRecoveryDir; http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tablet/tablet_peer.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/tablet_peer.cc b/src/kudu/tablet/tablet_peer.cc index 93301c2..24c720f 100644 --- a/src/kudu/tablet/tablet_peer.cc +++ b/src/kudu/tablet/tablet_peer.cc @@ -111,7 +111,7 @@ TabletPeer::TabletPeer(const scoped_refptr<TabletMetadata>& meta, tablet_id_(meta->tablet_id()), local_peer_pb_(local_peer_pb), state_(NOT_STARTED), - status_listener_(new TabletStatusListener(meta)), + last_status_("Tablet initializing..."), apply_pool_(apply_pool), log_anchor_registry_(new LogAnchorRegistry()), mark_dirty_clbk_(std::move(mark_dirty_clbk)) {} @@ -349,11 +349,10 @@ Status TabletPeer::SubmitAlterSchema(unique_ptr<AlterSchemaTransactionState> sta void TabletPeer::GetTabletStatusPB(TabletStatusPB* status_pb_out) const { std::lock_guard<simple_spinlock> lock(lock_); DCHECK(status_pb_out != nullptr); - DCHECK(status_listener_.get() != nullptr); - status_pb_out->set_tablet_id(status_listener_->tablet_id()); - status_pb_out->set_table_name(status_listener_->table_name()); - status_pb_out->set_last_status(status_listener_->last_status()); - status_listener_->partition().ToPB(status_pb_out->mutable_partition()); + status_pb_out->set_tablet_id(meta_->tablet_id()); + status_pb_out->set_table_name(meta_->table_name()); + status_pb_out->set_last_status(last_status_); + meta_->partition().ToPB(status_pb_out->mutable_partition()); status_pb_out->set_state(state_); status_pb_out->set_tablet_data_state(meta_->tablet_data_state()); if (tablet_) { @@ -376,11 +375,21 @@ Status TabletPeer::RunLogGC() { return Status::OK(); } +void TabletPeer::StatusMessage(const std::string& status) { + std::lock_guard<simple_spinlock> lock(lock_); + last_status_ = status; +} + +string TabletPeer::last_status() const { + std::lock_guard<simple_spinlock> lock(lock_); + return last_status_; +} + void TabletPeer::SetFailed(const Status& error) { std::lock_guard<simple_spinlock> lock(lock_); state_ = FAILED; error_ = error; - status_listener_->StatusMessage(error.ToString()); + last_status_ = error.ToString(); } string TabletPeer::HumanReadableState() const { http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tablet/tablet_peer.h ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/tablet_peer.h b/src/kudu/tablet/tablet_peer.h index 101c183..8658bf0 100644 --- a/src/kudu/tablet/tablet_peer.h +++ b/src/kudu/tablet/tablet_peer.h @@ -60,13 +60,24 @@ class TabletStatusPB; class TabletStatusListener; class TransactionDriver; +// Interface by which various tablet-related processes can report back their status +// to TabletPeer without having to have a circular class dependency, and so that +// those other classes can be easily tested without constructing a TabletPeer. +class TabletStatusListener { + public: + virtual ~TabletStatusListener() {} + + virtual void StatusMessage(const std::string& status) = 0; +}; + // A peer in a tablet consensus configuration, which coordinates writes to tablets. // Each time Write() is called this class appends a new entry to a replicated // state machine through a consensus algorithm, which makes sure that other // peers see the same updates in the same order. In addition to this, this // class also splits the work and coordinates multi-threaded execution. class TabletPeer : public RefCountedThreadSafe<TabletPeer>, - public consensus::ReplicaTransactionFactory { + public consensus::ReplicaTransactionFactory, + public TabletStatusListener { public: typedef std::map<int64_t, int64_t> MaxIdxToSegmentSizeMap; @@ -157,10 +168,6 @@ class TabletPeer : public RefCountedThreadSafe<TabletPeer>, // TODO: move this to raft_consensus.h. Status UpdatePermanentUuids(); - TabletStatusListener* status_listener() const { - return status_listener_.get(); - } - // Sets the tablet to a BOOTSTRAPPING state, indicating it is starting up. void SetBootstrapping() { std::lock_guard<simple_spinlock> lock(lock_); @@ -168,6 +175,12 @@ class TabletPeer : public RefCountedThreadSafe<TabletPeer>, state_ = BOOTSTRAPPING; } + // Implementation of TabletStatusListener::StatusMessage(). + void StatusMessage(const std::string& status) override; + + // Retrieve the last human-readable status of this tablet peer. + std::string last_status() const; + // Sets the tablet state to FAILED additionally setting the error to the provided // one. void SetFailed(const Status& error); @@ -287,13 +300,16 @@ class TabletPeer : public RefCountedThreadSafe<TabletPeer>, std::shared_ptr<Tablet> tablet_; std::shared_ptr<rpc::Messenger> messenger_; scoped_refptr<consensus::Consensus> consensus_; - gscoped_ptr<TabletStatusListener> status_listener_; simple_spinlock prepare_replicate_lock_; - // Lock protecting state_ as well as smart pointers to collaborating + // Lock protecting state_, last_status_, as well as smart pointers to collaborating // classes such as tablet_ and consensus_. mutable simple_spinlock lock_; + // The human-readable last status of the tablet, displayed on the web page, command line + // tools, etc. + std::string last_status_; + // Lock taken during Init/Shutdown which ensures that only a single thread // attempts to perform major lifecycle operations (Init/Shutdown) at once. // This must be acquired before acquiring lock_ if they are acquired together. http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tserver/tablet_copy_client-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/tablet_copy_client-test.cc b/src/kudu/tserver/tablet_copy_client-test.cc index 5fd3a6c..454833b 100644 --- a/src/kudu/tserver/tablet_copy_client-test.cc +++ b/src/kudu/tserver/tablet_copy_client-test.cc @@ -30,7 +30,6 @@ namespace tserver { using consensus::GetRaftConfigLeader; using consensus::RaftPeerPB; using tablet::TabletMetadata; -using tablet::TabletStatusListener; class TabletCopyClientTest : public TabletCopyTest { public: @@ -92,14 +91,12 @@ Status TabletCopyClientTest::CompareFileContents(const string& path1, const stri // Basic begin / end tablet copy session. TEST_F(TabletCopyClientTest, TestBeginEndSession) { - TabletStatusListener listener(meta_); - ASSERT_OK(client_->FetchAll(&listener)); + ASSERT_OK(client_->FetchAll(nullptr /* no listener */)); ASSERT_OK(client_->Finish()); } // Basic data block download unit test. TEST_F(TabletCopyClientTest, TestDownloadBlock) { - TabletStatusListener listener(meta_); BlockId block_id = FirstColumnBlockId(*client_->superblock_); Slice slice; faststring scratch; http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tserver/tablet_server-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc index eb24ccd..24cc182 100644 --- a/src/kudu/tserver/tablet_server-test.cc +++ b/src/kudu/tserver/tablet_server-test.cc @@ -994,7 +994,7 @@ TEST_F(TabletServerTest, TestClientGetsErrorBackWhenRecoveryFailed) { ASSERT_STR_CONTAINS(resp.error().status().message(), "Tablet not RUNNING: FAILED"); // Check that the tablet peer's status message is updated with the failure. - ASSERT_STR_CONTAINS(tablet_peer_->status_listener()->last_status(), + ASSERT_STR_CONTAINS(tablet_peer_->last_status(), "Log file corruption detected"); } http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tserver/tablet_service.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc index 8b9d770..fb3443e 100644 --- a/src/kudu/tserver/tablet_service.cc +++ b/src/kudu/tserver/tablet_service.cc @@ -1120,7 +1120,7 @@ void TabletServiceImpl::ListTablets(const ListTabletsRequestPB* req, peer->GetTabletStatusPB(status->mutable_tablet_status()); if (req->need_schema_info()) { - CHECK_OK(SchemaToPB(peer->status_listener()->schema(), + CHECK_OK(SchemaToPB(peer->tablet_metadata()->schema(), status->mutable_schema())); peer->tablet_metadata()->partition_schema().ToPB(status->mutable_partition_schema()); } http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tserver/ts_tablet_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/ts_tablet_manager.cc b/src/kudu/tserver/ts_tablet_manager.cc index bdc65dd..0e8b52a 100644 --- a/src/kudu/tserver/ts_tablet_manager.cc +++ b/src/kudu/tserver/ts_tablet_manager.cc @@ -33,6 +33,7 @@ #include "kudu/consensus/opid_util.h" #include "kudu/consensus/quorum_util.h" #include "kudu/fs/fs_manager.h" +#include "kudu/gutil/casts.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/gutil/strings/util.h" #include "kudu/master/master.pb.h" @@ -434,7 +435,8 @@ Status TSTabletManager::StartTabletCopy( string peer_str = copy_source_uuid + " (" + copy_source_addr.ToString() + ")"; // Download all of the remote files. - TOMBSTONE_NOT_OK(tc_client.FetchAll(tablet_peer->status_listener()), meta, + Status s = tc_client.FetchAll(implicit_cast<TabletStatusListener*>(tablet_peer.get())); + TOMBSTONE_NOT_OK(s, meta, "Tablet Copy: Unable to fetch data from remote peer " + copy_source_uuid + " (" + copy_source_addr.ToString() + ")"); @@ -544,7 +546,7 @@ Status TSTabletManager::DeleteTablet( return s; } - tablet_peer->status_listener()->StatusMessage("Deleted tablet blocks from disk"); + tablet_peer->StatusMessage("Deleted tablet blocks from disk"); // We only remove DELETED tablets from the tablet map. if (delete_type == TABLET_DATA_DELETED) { @@ -645,7 +647,7 @@ void TSTabletManager::OpenTablet(const scoped_refptr<TabletMetadata>& meta, server_->mem_tracker(), server_->result_tracker(), metric_registry_, - tablet_peer->status_listener(), + implicit_cast<TabletStatusListener*>(tablet_peer.get()), &tablet, &log, tablet_peer->log_anchor_registry(), http://git-wip-us.apache.org/repos/asf/kudu/blob/e1f54cbe/src/kudu/tserver/tserver-path-handlers.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/tserver-path-handlers.cc b/src/kudu/tserver/tserver-path-handlers.cc index aedf41a..962fa55 100644 --- a/src/kudu/tserver/tserver-path-handlers.cc +++ b/src/kudu/tserver/tserver-path-handlers.cc @@ -208,7 +208,7 @@ void TabletServerPathHandlers::HandleTabletsPage(const Webserver::WebRequest& re } string partition = peer->tablet_metadata() ->partition_schema() - .PartitionDebugString(peer->status_listener()->partition(), + .PartitionDebugString(peer->tablet_metadata()->partition(), peer->tablet_metadata()->schema()); // TODO: would be nice to include some other stuff like memory usage
