You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2016/06/10 02:25:50 UTC
incubator-kudu git commit: KUDU-1473: fix some tablet lock usage in
CatalogManager
Repository: incubator-kudu
Updated Branches:
refs/heads/master 5f7823fe7 -> 679c6b2c9
KUDU-1473: fix some tablet lock usage in CatalogManager
This was probably due to the refactoring done in commit 59ff89d. Now that
we're releasing tablet write locks as early as possible, we need to
reacquire them (in read mode) for some operations.
The lock misuse reared its head most often in the Java test
TestKuduTable.testGetLocations, but could trigger in just about any test
that didn't wait for table creation to finish before accessing the table.
For example, CreateTableITest.TestCreateWhenMajorityOfReplicasFailCreation
was a little flaky too. The backtrace was always the same:
cow_object.h:82] Check failed: lock_.HasReaders() || lock_.HasWriteLock()
@ 0x7fecb643e37b kudu::CowObject<>::state() at ??:0
@ 0x7fecb6422fe9 kudu::master::CatalogManager::ProcessPendingAssignments() at ??:0
@ 0x7fecb6421d3a kudu::master::CatalogManagerBgTasks::Run() at ??:0
@ 0x7fecb645c1f7 boost::_mfi::mf0<>::operator()() at ??:0
@ 0x7fecb645c15b boost::_bi::list1<>::operator()<>() at ??:0
@ 0x7fecb645c104 boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7fecb645bf2a boost::detail::function::void_function_obj_invoker0<>::invoke() at ??:0
@ 0x7fecb0b66d82 boost::function0<>::operator()() at ??:0
@ 0x7fecafba96c0 kudu::Thread::SuperviseThread() at ??:0
@ 0x423faa __tsan_thread_start_func at ??:0
@ 0x7fecb28459d1 start_thread at ??:0
@ 0x7fecacd108fd clone at ??:0
Change-Id: I8e24f6035f4d778995ea3f295396f5fbd760d6c6
Reviewed-on: http://gerrit.cloudera.org:8080/3309
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Adar Dembo <ad...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/679c6b2c
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/679c6b2c
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/679c6b2c
Branch: refs/heads/master
Commit: 679c6b2c93313bd4371916678b4ea8f52b621a50
Parents: 5f7823f
Author: Adar Dembo <ad...@cloudera.com>
Authored: Fri Jun 3 17:01:45 2016 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Fri Jun 10 02:24:15 2016 +0000
----------------------------------------------------------------------
src/kudu/master/catalog_manager.cc | 56 +++++++++++++++++++--------------
src/kudu/master/catalog_manager.h | 11 +++++--
2 files changed, 41 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/679c6b2c/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 7619759..ecdf412 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -2099,26 +2099,29 @@ class RetrySpecificTSRpcTask : public RetryingTSRpcTask {
// consensus configuration information has been filled into the 'dirty' data.
class AsyncCreateReplica : public RetrySpecificTSRpcTask {
public:
+
+ // The tablet lock must be acquired for reading before making this call.
AsyncCreateReplica(Master *master,
ThreadPool *callback_pool,
const string& permanent_uuid,
- const scoped_refptr<TabletInfo>& tablet)
+ const scoped_refptr<TabletInfo>& tablet,
+ const TabletMetadataLock& tablet_lock)
: RetrySpecificTSRpcTask(master, callback_pool, permanent_uuid, tablet->table().get()),
tablet_id_(tablet->tablet_id()) {
deadline_ = start_ts_;
deadline_.AddDelta(MonoDelta::FromMilliseconds(FLAGS_tablet_creation_timeout_ms));
TableMetadataLock table_lock(tablet->table().get(), TableMetadataLock::READ);
- const SysTabletsEntryPB& tablet_pb = tablet->metadata().state().pb;
-
req_.set_dest_uuid(permanent_uuid);
req_.set_table_id(tablet->table()->id());
req_.set_tablet_id(tablet->tablet_id());
- req_.mutable_partition()->CopyFrom(tablet_pb.partition());
+ req_.mutable_partition()->CopyFrom(tablet_lock.data().pb.partition());
req_.set_table_name(table_lock.data().pb.name());
req_.mutable_schema()->CopyFrom(table_lock.data().pb.schema());
- req_.mutable_partition_schema()->CopyFrom(table_lock.data().pb.partition_schema());
- req_.mutable_config()->CopyFrom(tablet_pb.committed_consensus_state().config());
+ req_.mutable_partition_schema()->CopyFrom(
+ table_lock.data().pb.partition_schema());
+ req_.mutable_config()->CopyFrom(
+ tablet_lock.data().pb.committed_consensus_state().config());
}
virtual string type_name() const OVERRIDE { return "Create Tablet"; }
@@ -2502,14 +2505,15 @@ void CatalogManager::SendDeleteTableRequest(const scoped_refptr<TableInfo>& tabl
table->GetAllTablets(&tablets);
for (const scoped_refptr<TabletInfo>& tablet : tablets) {
- SendDeleteTabletRequest(tablet, deletion_msg);
+ TabletMetadataLock l(tablet.get(), TabletMetadataLock::READ);
+ SendDeleteTabletRequest(tablet, l, deletion_msg);
}
}
void CatalogManager::SendDeleteTabletRequest(const scoped_refptr<TabletInfo>& tablet,
+ const TabletMetadataLock& tablet_lock,
const string& deletion_msg) {
- TabletMetadataLock l(tablet.get(), TabletMetadataLock::READ);
- if (!l.data().pb.has_committed_consensus_state()) {
+ if (!tablet_lock.data().pb.has_committed_consensus_state()) {
// We could end up here if we're deleting a tablet that never made it to
// the CREATING state. That would mean no replicas were ever assigned, so
// there's nothing to delete.
@@ -2517,7 +2521,7 @@ void CatalogManager::SendDeleteTabletRequest(const scoped_refptr<TabletInfo>& ta
<< tablet->tablet_id();
return;
}
- const ConsensusStatePB& cstate = l.data().pb.committed_consensus_state();
+ const ConsensusStatePB& cstate = tablet_lock.data().pb.committed_consensus_state();
LOG(INFO) << "Sending DeleteTablet for " << cstate.config().peers().size()
<< " replicas of tablet " << tablet->tablet_id();
for (const auto& peer : cstate.config().peers()) {
@@ -2797,12 +2801,16 @@ Status CatalogManager::ProcessPendingAssignments(
// Send DeleteTablet requests to tablet servers serving deleted tablets.
// This is asynchronous / non-blocking.
for (TabletInfo* tablet : deferred.tablets_to_update) {
- if (tablet->metadata().state().is_deleted()) {
- SendDeleteTabletRequest(tablet, tablet->metadata().state().pb.state_msg());
+ TabletMetadataLock l(tablet, TabletMetadataLock::READ);
+ if (l.data().is_deleted()) {
+ SendDeleteTabletRequest(tablet, l, l.data().pb.state_msg());
}
}
// Send the CreateTablet() requests to the servers. This is asynchronous / non-blocking.
- SendCreateTabletRequests(deferred.needs_create_rpc);
+ for (TabletInfo* tablet : deferred.needs_create_rpc) {
+ TabletMetadataLock l(tablet, TabletMetadataLock::READ);
+ SendCreateTabletRequest(tablet, l);
+ }
return Status::OK();
}
@@ -2841,17 +2849,17 @@ Status CatalogManager::SelectReplicasForTablet(const TSDescriptorVector& ts_desc
return Status::OK();
}
-void CatalogManager::SendCreateTabletRequests(const vector<TabletInfo*>& tablets) {
- for (TabletInfo *tablet : tablets) {
- const consensus::RaftConfigPB& config =
- tablet->metadata().state().pb.committed_consensus_state().config();
- tablet->set_last_create_tablet_time(MonoTime::Now(MonoTime::FINE));
- for (const RaftPeerPB& peer : config.peers()) {
- AsyncCreateReplica* task = new AsyncCreateReplica(master_, worker_pool_.get(),
- peer.permanent_uuid(), tablet);
- tablet->table()->AddTask(task);
- WARN_NOT_OK(task->Run(), "Failed to send new tablet request");
- }
+void CatalogManager::SendCreateTabletRequest(const scoped_refptr<TabletInfo>& tablet,
+ const TabletMetadataLock& tablet_lock) {
+ const consensus::RaftConfigPB& config =
+ tablet_lock.data().pb.committed_consensus_state().config();
+ tablet->set_last_create_tablet_time(MonoTime::Now(MonoTime::FINE));
+ for (const RaftPeerPB& peer : config.peers()) {
+ AsyncCreateReplica* task = new AsyncCreateReplica(master_, worker_pool_.get(),
+ peer.permanent_uuid(),
+ tablet, tablet_lock);
+ tablet->table()->AddTask(task);
+ WARN_NOT_OK(task->Run(), "Failed to send new tablet request");
}
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/679c6b2c/src/kudu/master/catalog_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.h b/src/kudu/master/catalog_manager.h
index 586be9e..1cc52f0 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -522,7 +522,8 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
Status HandleTabletSchemaVersionReport(TabletInfo *tablet,
uint32_t version);
- // Send the create tablet requests to the selected peers of the consensus configurations.
+ // Send the "create tablet request" to all peers of a particular tablet.
+ //.
// The creation is async, and at the moment there is no error checking on the
// caller side. We rely on the assignment timeout. If we don't see the tablet
// after the timeout, we regenerate a new one and proceed with a new
@@ -532,7 +533,10 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
//
// This must be called after persisting the tablet state as
// CREATING to ensure coherent state after Master failover.
- void SendCreateTabletRequests(const std::vector<TabletInfo*>& tablets);
+ //
+ // The tablet lock must be acquired for reading before making this call.
+ void SendCreateTabletRequest(const scoped_refptr<TabletInfo>& tablet,
+ const TabletMetadataLock& tablet_lock);
// Send the "alter table request" to all tablets of the specified table.
void SendAlterTableRequest(const scoped_refptr<TableInfo>& table);
@@ -547,7 +551,10 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
const std::string& deletion_msg);
// Send the "delete tablet request" to all replicas of the specified tablet.
+ //
+ // The tablet lock must be acquired for reading before making this call.
void SendDeleteTabletRequest(const scoped_refptr<TabletInfo>& tablet,
+ const TabletMetadataLock& tablet_lock,
const std::string& deletion_msg);
// Send the "delete tablet request" to a particular replica (i.e. TS and