You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2016/07/18 23:13:06 UTC
[2/3] incubator-kudu git commit: master: add assert checks for
leader_lock
master: add assert checks for leader_lock
A side effect of recursive checking in RWMutex is that we can now assert
that a RWMutex is held for reading/writing. Let's add that to the various
catalog manager entry points.
Change-Id: Iefb5762c70192b27490cc71e20568815d18d6ad5
Reviewed-on: http://gerrit.cloudera.org:8080/3642
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@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/0a106063
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/0a106063
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/0a106063
Branch: refs/heads/master
Commit: 0a10606379b08b731ef680afd2291e44c671315a
Parents: 2a3d898
Author: Adar Dembo <ad...@cloudera.com>
Authored: Wed Jul 13 15:23:37 2016 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Mon Jul 18 23:01:00 2016 +0000
----------------------------------------------------------------------
src/kudu/client/client-test.cc | 7 ++++--
src/kudu/integration-tests/alter_table-test.cc | 12 ++++++----
.../create-table-stress-test.cc | 17 +++++++++-----
src/kudu/integration-tests/mini_cluster.cc | 17 +++++++-------
src/kudu/integration-tests/mini_cluster.h | 7 ------
src/kudu/master/catalog_manager.cc | 24 +++++++++++++-------
src/kudu/master/catalog_manager.h | 9 ++++++++
src/kudu/master/master-test-util.h | 24 ++++++++++++++++----
src/kudu/master/master.cc | 2 +-
9 files changed, 78 insertions(+), 41 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/client/client-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index 9d26b8b..404c7b7 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -173,8 +173,11 @@ class ClientTest : public KuduTest {
GetTableLocationsRequestPB req;
GetTableLocationsResponsePB resp;
req.mutable_table()->set_table_name(table->name());
- CHECK_OK(cluster_->mini_master()->master()->catalog_manager()->GetTableLocations(
- &req, &resp));
+ CatalogManager* catalog =
+ cluster_->mini_master()->master()->catalog_manager();
+ CatalogManager::ScopedLeaderSharedLock l(catalog);
+ CHECK_OK(l.first_failed_status());
+ CHECK_OK(catalog->GetTableLocations(&req, &resp));
CHECK(resp.tablet_locations_size() > 0);
return resp.tablet_locations(0).tablet_id();
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/integration-tests/alter_table-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc
index c876f7e..8c97a39 100644
--- a/src/kudu/integration-tests/alter_table-test.cc
+++ b/src/kudu/integration-tests/alter_table-test.cc
@@ -294,15 +294,19 @@ TEST_F(AlterTableTest, TestAddNotNullableColumnWithoutDefaults) {
{
AlterTableRequestPB req;
- req.mutable_table()->set_table_name(kTableName);
+ AlterTableResponsePB resp;
+ req.mutable_table()->set_table_name(kTableName);
AlterTableRequestPB::Step *step = req.add_alter_schema_steps();
step->set_type(AlterTableRequestPB::ADD_COLUMN);
ColumnSchemaToPB(ColumnSchema("c2", INT32),
step->mutable_add_column()->mutable_schema());
- AlterTableResponsePB resp;
- Status s = cluster_->mini_master()->master()->catalog_manager()->AlterTable(
- &req, &resp, nullptr);
+
+ master::CatalogManager* catalog =
+ cluster_->mini_master()->master()->catalog_manager();
+ master::CatalogManager::ScopedLeaderSharedLock l(catalog);
+ ASSERT_OK(l.first_failed_status());
+ Status s = catalog->AlterTable(&req, &resp, nullptr);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_STR_CONTAINS(s.ToString(), "column `c2`: NOT NULL columns must have a default");
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/integration-tests/create-table-stress-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/create-table-stress-test.cc b/src/kudu/integration-tests/create-table-stress-test.cc
index c740e58..c4c63f4 100644
--- a/src/kudu/integration-tests/create-table-stress-test.cc
+++ b/src/kudu/integration-tests/create-table-stress-test.cc
@@ -222,6 +222,11 @@ TEST_F(CreateTableStressTest, TestGetTableLocationsOptions) {
FLAGS_num_test_tablets, &resp));
}
+ master::CatalogManager* catalog =
+ cluster_->mini_master()->master()->catalog_manager();
+ master::CatalogManager::ScopedLeaderSharedLock l(catalog);
+ ASSERT_OK(l.first_failed_status());
+
// Test asking for 0 tablets, should fail
LOG(INFO) << CURRENT_TEST_NAME() << ": Step 3. Asking for zero tablets...";
LOG_TIMING(INFO, "asking for zero tablets") {
@@ -229,7 +234,7 @@ TEST_F(CreateTableStressTest, TestGetTableLocationsOptions) {
resp.Clear();
req.mutable_table()->set_table_name(table_name);
req.set_max_returned_locations(0);
- Status s = cluster_->mini_master()->master()->catalog_manager()->GetTableLocations(&req, &resp);
+ Status s = catalog->GetTableLocations(&req, &resp);
ASSERT_STR_CONTAINS(s.ToString(), "must be greater than 0");
}
@@ -240,7 +245,7 @@ TEST_F(CreateTableStressTest, TestGetTableLocationsOptions) {
resp.Clear();
req.mutable_table()->set_table_name(table_name);
req.set_max_returned_locations(1);
- ASSERT_OK(cluster_->mini_master()->master()->catalog_manager()->GetTableLocations(&req, &resp));
+ ASSERT_OK(catalog->GetTableLocations(&req, &resp));
ASSERT_EQ(resp.tablet_locations_size(), 1);
// empty since it's the first
ASSERT_EQ(resp.tablet_locations(0).partition().partition_key_start(), "");
@@ -255,7 +260,7 @@ TEST_F(CreateTableStressTest, TestGetTableLocationsOptions) {
resp.Clear();
req.mutable_table()->set_table_name(table_name);
req.set_max_returned_locations(half_tablets);
- ASSERT_OK(cluster_->mini_master()->master()->catalog_manager()->GetTableLocations(&req, &resp));
+ ASSERT_OK(catalog->GetTableLocations(&req, &resp));
ASSERT_EQ(half_tablets, resp.tablet_locations_size());
}
@@ -266,7 +271,7 @@ TEST_F(CreateTableStressTest, TestGetTableLocationsOptions) {
resp.Clear();
req.mutable_table()->set_table_name(table_name);
req.set_max_returned_locations(FLAGS_num_test_tablets);
- ASSERT_OK(cluster_->mini_master()->master()->catalog_manager()->GetTableLocations(&req, &resp));
+ ASSERT_OK(catalog->GetTableLocations(&req, &resp));
ASSERT_EQ(FLAGS_num_test_tablets, resp.tablet_locations_size());
}
@@ -274,7 +279,7 @@ TEST_F(CreateTableStressTest, TestGetTableLocationsOptions) {
LOG(INFO) << "Tables and tablets:";
LOG(INFO) << "========================================================";
std::vector<scoped_refptr<master::TableInfo> > tables;
- cluster_->mini_master()->master()->catalog_manager()->GetAllTables(&tables);
+ catalog->GetAllTables(&tables);
for (const scoped_refptr<master::TableInfo>& table_info : tables) {
LOG(INFO) << "Table: " << table_info->ToString();
std::vector<scoped_refptr<master::TabletInfo> > tablets;
@@ -310,7 +315,7 @@ TEST_F(CreateTableStressTest, TestGetTableLocationsOptions) {
req.mutable_table()->set_table_name(table_name);
req.set_max_returned_locations(1);
req.set_partition_key_start(start_key_middle);
- ASSERT_OK(cluster_->mini_master()->master()->catalog_manager()->GetTableLocations(&req, &resp));
+ ASSERT_OK(catalog->GetTableLocations(&req, &resp));
ASSERT_EQ(1, resp.tablet_locations_size()) << "Response: [" << resp.DebugString() << "]";
ASSERT_EQ(start_key_middle, resp.tablet_locations(0).partition().partition_key_start());
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/integration-tests/mini_cluster.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/mini_cluster.cc b/src/kudu/integration-tests/mini_cluster.cc
index 7133b8e..2b5ef67 100644
--- a/src/kudu/integration-tests/mini_cluster.cc
+++ b/src/kudu/integration-tests/mini_cluster.cc
@@ -190,7 +190,7 @@ MiniMaster* MiniCluster::leader_mini_master() {
}
CatalogManager::ScopedLeaderSharedLock l(
master->master()->catalog_manager());
- if (l.catalog_status().ok() && l.leader_status().ok()) {
+ if (l.first_failed_status().ok()) {
return master;
}
}
@@ -241,19 +241,18 @@ string MiniCluster::GetTabletServerFsRoot(int idx) {
}
Status MiniCluster::WaitForReplicaCount(const string& tablet_id,
- int expected_count) {
- TabletLocationsPB locations;
- return WaitForReplicaCount(tablet_id, expected_count, &locations);
-}
-
-Status MiniCluster::WaitForReplicaCount(const string& tablet_id,
int expected_count,
TabletLocationsPB* locations) {
Stopwatch sw;
sw.start();
while (sw.elapsed().wall_seconds() < kTabletReportWaitTimeSeconds) {
- Status s =
- leader_mini_master()->master()->catalog_manager()->GetTabletLocations(tablet_id, locations);
+ CatalogManager* catalog = leader_mini_master()->master()->catalog_manager();
+ Status s;
+ {
+ CatalogManager::ScopedLeaderSharedLock l(catalog);
+ RETURN_NOT_OK(l.first_failed_status());
+ s = catalog->GetTabletLocations(tablet_id, locations);
+ }
if (s.ok() && locations->replicas_size() == expected_count) {
return Status::OK();
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/integration-tests/mini_cluster.h
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/mini_cluster.h b/src/kudu/integration-tests/mini_cluster.h
index 40e4af2..e682316 100644
--- a/src/kudu/integration-tests/mini_cluster.h
+++ b/src/kudu/integration-tests/mini_cluster.h
@@ -130,13 +130,6 @@ class MiniCluster {
std::string GetTabletServerFsRoot(int idx);
// Wait for the given tablet to have 'expected_count' replicas
- // reported on the master.
- // Requires that the master has started.
- // Returns a bad Status if the tablet does not reach the required count
- // within kTabletReportWaitTimeSeconds.
- Status WaitForReplicaCount(const std::string& tablet_id, int expected_count);
-
- // Wait for the given tablet to have 'expected_count' replicas
// reported on the master. Returns the locations in '*locations'.
// Requires that the master has started;
// Returns a bad Status if the tablet does not reach the required count
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 88bd3e5..9271436 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -754,6 +754,7 @@ Status CatalogManager::CheckOnline() const {
Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
CreateTableResponsePB* resp,
rpc::RpcContext* rpc) {
+ leader_lock_.AssertAcquiredForReading();
RETURN_NOT_OK(CheckOnline());
Status s;
@@ -954,6 +955,7 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
Status CatalogManager::IsCreateTableDone(const IsCreateTableDoneRequestPB* req,
IsCreateTableDoneResponsePB* resp) {
+ leader_lock_.AssertAcquiredForReading();
RETURN_NOT_OK(CheckOnline());
scoped_refptr<TableInfo> table;
@@ -1025,11 +1027,12 @@ Status CatalogManager::FindTable(const TableIdentifierPB& table_identifier,
Status CatalogManager::DeleteTable(const DeleteTableRequestPB* req,
DeleteTableResponsePB* resp,
rpc::RpcContext* rpc) {
+ leader_lock_.AssertAcquiredForReading();
+ RETURN_NOT_OK(CheckOnline());
+
LOG(INFO) << "Servicing DeleteTable request from " << RequestorString(rpc)
<< ": " << req->ShortDebugString();
- RETURN_NOT_OK(CheckOnline());
-
// 1. Look up the table, lock it, and mark it as removed.
TRACE("Looking up table");
scoped_refptr<TableInfo> table;
@@ -1201,11 +1204,12 @@ static Status ApplyAlterSteps(const SysTablesEntryPB& current_pb,
Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
AlterTableResponsePB* resp,
rpc::RpcContext* rpc) {
+ leader_lock_.AssertAcquiredForReading();
+ RETURN_NOT_OK(CheckOnline());
+
LOG(INFO) << "Servicing AlterTable request from " << RequestorString(rpc)
<< ": " << req->ShortDebugString();
- RETURN_NOT_OK(CheckOnline());
-
// 1. Lookup the table and verify if it exists.
TRACE("Looking up table");
scoped_refptr<TableInfo> table;
@@ -1345,6 +1349,7 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
Status CatalogManager::IsAlterTableDone(const IsAlterTableDoneRequestPB* req,
IsAlterTableDoneResponsePB* resp,
rpc::RpcContext* rpc) {
+ leader_lock_.AssertAcquiredForReading();
RETURN_NOT_OK(CheckOnline());
scoped_refptr<TableInfo> table;
@@ -1372,6 +1377,7 @@ Status CatalogManager::IsAlterTableDone(const IsAlterTableDoneRequestPB* req,
Status CatalogManager::GetTableSchema(const GetTableSchemaRequestPB* req,
GetTableSchemaResponsePB* resp) {
+ leader_lock_.AssertAcquiredForReading();
RETURN_NOT_OK(CheckOnline());
scoped_refptr<TableInfo> table;
@@ -1409,6 +1415,7 @@ Status CatalogManager::GetTableSchema(const GetTableSchemaRequestPB* req,
Status CatalogManager::ListTables(const ListTablesRequestPB* req,
ListTablesResponsePB* resp) {
+ leader_lock_.AssertAcquiredForReading();
RETURN_NOT_OK(CheckOnline());
shared_lock<LockType> l(lock_);
@@ -1466,6 +1473,8 @@ Status CatalogManager::ProcessTabletReport(TSDescriptor* ts_desc,
"requestor", rpc->requestor_string(),
"num_tablets", report.updated_tablets_size());
+ leader_lock_.AssertAcquiredForReading();
+
if (VLOG_IS_ON(2)) {
VLOG(2) << "Received tablet report from " <<
RequestorString(rpc) << ": " << report.DebugString();
@@ -3018,6 +3027,7 @@ Status CatalogManager::BuildLocationsForTablet(const scoped_refptr<TabletInfo>&
Status CatalogManager::GetTabletLocations(const std::string& tablet_id,
TabletLocationsPB* locs_pb) {
+ leader_lock_.AssertAcquiredForReading();
RETURN_NOT_OK(CheckOnline());
locs_pb->mutable_replicas()->Clear();
@@ -3034,6 +3044,7 @@ Status CatalogManager::GetTabletLocations(const std::string& tablet_id,
Status CatalogManager::GetTableLocations(const GetTableLocationsRequestPB* req,
GetTableLocationsResponsePB* resp) {
+ leader_lock_.AssertAcquiredForReading();
RETURN_NOT_OK(CheckOnline());
// If start-key is > end-key report an error instead of swap the two
@@ -3214,10 +3225,7 @@ bool CatalogManager::ScopedLeaderSharedLock::CheckIsInitializedOrRespond(
template<typename RespClass>
bool CatalogManager::ScopedLeaderSharedLock::CheckIsInitializedAndIsLeaderOrRespond(
RespClass* resp, RpcContext* rpc) {
- Status& s = catalog_status_;
- if (PREDICT_TRUE(s.ok())) {
- s = leader_status_;
- }
+ const Status& s = first_failed_status();
if (PREDICT_TRUE(s.ok())) {
return true;
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/master/catalog_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.h b/src/kudu/master/catalog_manager.h
index 44f84c0..0196d8d 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -313,6 +313,15 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
return leader_status_;
}
+ // First non-OK status of the catalog manager, adhering to the checking
+ // order specified above.
+ const Status& first_failed_status() const {
+ if (!catalog_status_.ok()) {
+ return catalog_status_;
+ }
+ return leader_status_;
+ }
+
// Check that the catalog manager is initialized. It may or may not be the
// leader of its Raft configuration.
//
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/master/master-test-util.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/master-test-util.h b/src/kudu/master/master-test-util.h
index da5861b..dc8b0eb 100644
--- a/src/kudu/master/master-test-util.h
+++ b/src/kudu/master/master-test-util.h
@@ -46,7 +46,12 @@ Status WaitForRunningTabletCount(MiniMaster* mini_master,
resp->Clear();
req.mutable_table()->set_table_name(table_name);
req.set_max_returned_locations(expected_count);
- RETURN_NOT_OK(mini_master->master()->catalog_manager()->GetTableLocations(&req, resp));
+ CatalogManager* catalog = mini_master->master()->catalog_manager();
+ {
+ CatalogManager::ScopedLeaderSharedLock l(catalog);
+ RETURN_NOT_OK(l.first_failed_status());
+ RETURN_NOT_OK(catalog->GetTableLocations(&req, resp));
+ }
if (resp->tablet_locations_size() >= expected_count) {
return Status::OK();
}
@@ -74,7 +79,10 @@ void CreateTabletForTesting(MiniMaster* mini_master,
req.set_name(table_name);
req.set_num_replicas(1);
ASSERT_OK(SchemaToPB(schema, req.mutable_schema()));
- ASSERT_OK(mini_master->master()->catalog_manager()->CreateTable(&req, &resp, NULL));
+ CatalogManager* catalog = mini_master->master()->catalog_manager();
+ CatalogManager::ScopedLeaderSharedLock l(catalog);
+ ASSERT_OK(l.first_failed_status());
+ ASSERT_OK(catalog->CreateTable(&req, &resp, NULL));
}
int wait_time = 1000;
@@ -84,7 +92,12 @@ void CreateTabletForTesting(MiniMaster* mini_master,
IsCreateTableDoneResponsePB resp;
req.mutable_table()->set_table_name(table_name);
- ASSERT_OK(mini_master->master()->catalog_manager()->IsCreateTableDone(&req, &resp));
+ CatalogManager* catalog = mini_master->master()->catalog_manager();
+ {
+ CatalogManager::ScopedLeaderSharedLock l(catalog);
+ ASSERT_OK(l.first_failed_status());
+ ASSERT_OK(catalog->IsCreateTableDone(&req, &resp));
+ }
if (resp.done()) {
is_table_created = true;
break;
@@ -101,7 +114,10 @@ void CreateTabletForTesting(MiniMaster* mini_master,
GetTableSchemaRequestPB req;
GetTableSchemaResponsePB resp;
req.mutable_table()->set_table_name(table_name);
- ASSERT_OK(mini_master->master()->catalog_manager()->GetTableSchema(&req, &resp));
+ CatalogManager* catalog = mini_master->master()->catalog_manager();
+ CatalogManager::ScopedLeaderSharedLock l(catalog);
+ ASSERT_OK(l.first_failed_status());
+ ASSERT_OK(catalog->GetTableSchema(&req, &resp));
ASSERT_TRUE(resp.create_table_done());
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0a106063/src/kudu/master/master.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/master.cc b/src/kudu/master/master.cc
index c42d7f0..0910280 100644
--- a/src/kudu/master/master.cc
+++ b/src/kudu/master/master.cc
@@ -161,7 +161,7 @@ Status Master::WaitUntilCatalogManagerIsLeaderAndReadyForTests(const MonoDelta&
do {
{
CatalogManager::ScopedLeaderSharedLock l(catalog_manager_.get());
- if (l.catalog_status().ok() && l.leader_status().ok()) {
+ if (l.first_failed_status().ok()) {
return Status::OK();
}
}