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 2017/09/18 21:22:10 UTC
kudu git commit: c++ client: try harder to pass table IDs into RPCs
that can accept them
Repository: kudu
Updated Branches:
refs/heads/master 41a41fdf7 -> e171466e8
c++ client: try harder to pass table IDs into RPCs that can accept them
Some RPCs (such as IsCreateTableDone or IsAlterTableDone) can identify a
table by name or by ID. Using an ID is more robust since it's globally
unique and immutable. This patch changes several RPC calls to use table IDs.
Note: table IDs were only added to AlterTableResponsePB in Kudu 0.10. By
using them here, the C++ client may crash when altering tables belonging to
an older deployment.
Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Reviewed-on: http://gerrit.cloudera.org:8080/8066
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: David Ribeiro Alves <da...@gmail.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e171466e
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e171466e
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e171466e
Branch: refs/heads/master
Commit: e171466e8064b1c4d24d387eb51014ad77bb65fa
Parents: 41a41fd
Author: Adar Dembo <ad...@cloudera.com>
Authored: Wed Sep 13 21:13:01 2017 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Mon Sep 18 21:21:44 2017 +0000
----------------------------------------------------------------------
src/kudu/client/client-internal.cc | 52 ++++++++++----------
src/kudu/client/client-internal.h | 18 ++++---
src/kudu/client/client.cc | 36 ++++++++++----
src/kudu/client/client.h | 4 +-
src/kudu/integration-tests/alter_table-test.cc | 36 ++++++++++++++
.../integration-tests/master_failover-itest.cc | 27 +++++-----
6 files changed, 113 insertions(+), 60 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/e171466e/src/kudu/client/client-internal.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-internal.cc b/src/kudu/client/client-internal.cc
index 2a1b7a3..281e9a7 100644
--- a/src/kudu/client/client-internal.cc
+++ b/src/kudu/client/client-internal.cc
@@ -94,6 +94,7 @@ using master::ListTabletServersResponsePB;
using master::MasterErrorPB;
using master::MasterFeatures;
using master::MasterServiceProxy;
+using master::TableIdentifierPB;
using pb_util::SecureShortDebugString;
using rpc::CredentialsPolicy;
using rpc::ErrorStatusPB;
@@ -442,27 +443,26 @@ Status KuduClient::Data::GetTabletServer(KuduClient* client,
Status KuduClient::Data::CreateTable(KuduClient* client,
const CreateTableRequestPB& req,
- const KuduSchema& schema,
+ CreateTableResponsePB* resp,
const MonoTime& deadline,
bool has_range_partition_bounds) {
- CreateTableResponsePB resp;
-
vector<uint32_t> features;
if (has_range_partition_bounds) {
features.push_back(MasterFeatures::RANGE_PARTITION_BOUNDS);
}
return SyncLeaderMasterRpc<CreateTableRequestPB, CreateTableResponsePB>(
- deadline, client, req, &resp, "CreateTable",
+ deadline, client, req, resp, "CreateTable",
&MasterServiceProxy::CreateTable, features);
}
-Status KuduClient::Data::IsCreateTableInProgress(KuduClient* client,
- const string& table_name,
- const MonoTime& deadline,
- bool *create_in_progress) {
+Status KuduClient::Data::IsCreateTableInProgress(
+ KuduClient* client,
+ TableIdentifierPB table,
+ const MonoTime& deadline,
+ bool* create_in_progress) {
IsCreateTableDoneRequestPB req;
IsCreateTableDoneResponsePB resp;
- req.mutable_table()->set_table_name(table_name);
+ *req.mutable_table() = std::move(table);
// TODO(aserbin): Add client rpc timeout and use
// 'default_admin_operation_timeout_' as the default timeout for all
@@ -480,14 +480,15 @@ Status KuduClient::Data::IsCreateTableInProgress(KuduClient* client,
return Status::OK();
}
-Status KuduClient::Data::WaitForCreateTableToFinish(KuduClient* client,
- const string& table_name,
- const MonoTime& deadline) {
+Status KuduClient::Data::WaitForCreateTableToFinish(
+ KuduClient* client,
+ TableIdentifierPB table,
+ const MonoTime& deadline) {
return RetryFunc(deadline,
"Waiting on Create Table to be completed",
"Timed out waiting for Table Creation",
boost::bind(&KuduClient::Data::IsCreateTableInProgress,
- this, client, table_name, _1, _2));
+ this, client, std::move(table), _1, _2));
}
Status KuduClient::Data::DeleteTable(KuduClient* client,
@@ -504,31 +505,32 @@ Status KuduClient::Data::DeleteTable(KuduClient* client,
Status KuduClient::Data::AlterTable(KuduClient* client,
const AlterTableRequestPB& req,
+ AlterTableResponsePB* resp,
const MonoTime& deadline,
bool has_add_drop_partition) {
vector<uint32_t> required_feature_flags;
if (has_add_drop_partition) {
required_feature_flags.push_back(MasterFeatures::ADD_DROP_RANGE_PARTITIONS);
}
- AlterTableResponsePB resp;
return SyncLeaderMasterRpc<AlterTableRequestPB, AlterTableResponsePB>(
deadline,
client,
req,
- &resp,
+ resp,
"AlterTable",
&MasterServiceProxy::AlterTable,
std::move(required_feature_flags));
}
-Status KuduClient::Data::IsAlterTableInProgress(KuduClient* client,
- const string& table_name,
- const MonoTime& deadline,
- bool *alter_in_progress) {
+Status KuduClient::Data::IsAlterTableInProgress(
+ KuduClient* client,
+ TableIdentifierPB table,
+ const MonoTime& deadline,
+ bool* alter_in_progress) {
IsAlterTableDoneRequestPB req;
IsAlterTableDoneResponsePB resp;
+ *req.mutable_table() = std::move(table);
- req.mutable_table()->set_table_name(table_name);
RETURN_NOT_OK((
SyncLeaderMasterRpc<IsAlterTableDoneRequestPB, IsAlterTableDoneResponsePB>(
deadline,
@@ -542,15 +544,15 @@ Status KuduClient::Data::IsAlterTableInProgress(KuduClient* client,
return Status::OK();
}
-Status KuduClient::Data::WaitForAlterTableToFinish(KuduClient* client,
- const string& alter_name,
- const MonoTime& deadline) {
+Status KuduClient::Data::WaitForAlterTableToFinish(
+ KuduClient* client,
+ TableIdentifierPB table,
+ const MonoTime& deadline) {
return RetryFunc(deadline,
"Waiting on Alter Table to be completed",
"Timed out waiting for AlterTable",
boost::bind(&KuduClient::Data::IsAlterTableInProgress,
- this,
- client, alter_name, _1, _2));
+ this, client, std::move(table), _1, _2));
}
Status KuduClient::Data::InitLocalHostNames() {
http://git-wip-us.apache.org/repos/asf/kudu/blob/e171466e/src/kudu/client/client-internal.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-internal.h b/src/kudu/client/client-internal.h
index 44da7b9..a4ca44c 100644
--- a/src/kudu/client/client-internal.h
+++ b/src/kudu/client/client-internal.h
@@ -53,9 +53,12 @@ class Sockaddr;
namespace master {
class AlterTableRequestPB;
+class AlterTableResponsePB;
class ConnectToMasterResponsePB;
class CreateTableRequestPB;
+class CreateTableResponsePB;
class MasterServiceProxy;
+class TableIdentifierPB;
} // namespace master
namespace rpc {
@@ -96,17 +99,17 @@ class KuduClient::Data {
Status CreateTable(KuduClient* client,
const master::CreateTableRequestPB& req,
- const KuduSchema& schema,
+ master::CreateTableResponsePB* resp,
const MonoTime& deadline,
bool has_range_partition_bounds);
Status IsCreateTableInProgress(KuduClient* client,
- const std::string& table_name,
+ master::TableIdentifierPB table,
const MonoTime& deadline,
- bool *create_in_progress);
+ bool* create_in_progress);
Status WaitForCreateTableToFinish(KuduClient* client,
- const std::string& table_name,
+ master::TableIdentifierPB table,
const MonoTime& deadline);
Status DeleteTable(KuduClient* client,
@@ -115,16 +118,17 @@ class KuduClient::Data {
Status AlterTable(KuduClient* client,
const master::AlterTableRequestPB& req,
+ master::AlterTableResponsePB* resp,
const MonoTime& deadline,
bool has_add_drop_partition);
Status IsAlterTableInProgress(KuduClient* client,
- const std::string& table_name,
+ master::TableIdentifierPB table,
const MonoTime& deadline,
- bool *alter_in_progress);
+ bool* alter_in_progress);
Status WaitForAlterTableToFinish(KuduClient* client,
- const std::string& alter_name,
+ master::TableIdentifierPB table,
const MonoTime& deadline);
Status GetTableSchema(KuduClient* client,
http://git-wip-us.apache.org/repos/asf/kudu/blob/e171466e/src/kudu/client/client.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc
index f44cbb4..5cec57c 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -113,6 +113,7 @@ using kudu::master::ListTabletServersRequestPB;
using kudu::master::ListTabletServersResponsePB;
using kudu::master::ListTabletServersResponsePB_Entry;
using kudu::master::MasterServiceProxy;
+using kudu::master::TableIdentifierPB;
using kudu::master::TabletLocationsPB;
using kudu::master::TSInfoPB;
using kudu::rpc::Messenger;
@@ -364,9 +365,12 @@ KuduTableCreator* KuduClient::NewTableCreator() {
}
Status KuduClient::IsCreateTableInProgress(const string& table_name,
- bool *create_in_progress) {
+ bool* create_in_progress) {
MonoTime deadline = MonoTime::Now() + default_admin_operation_timeout();
- return data_->IsCreateTableInProgress(this, table_name, deadline, create_in_progress);
+ TableIdentifierPB table;
+ table.set_table_name(table_name);
+ return data_->IsCreateTableInProgress(this, std::move(table), deadline,
+ create_in_progress);
}
Status KuduClient::DeleteTable(const string& table_name) {
@@ -379,9 +383,12 @@ KuduTableAlterer* KuduClient::NewTableAlterer(const string& name) {
}
Status KuduClient::IsAlterTableInProgress(const string& table_name,
- bool *alter_in_progress) {
+ bool* alter_in_progress) {
MonoTime deadline = MonoTime::Now() + default_admin_operation_timeout();
- return data_->IsAlterTableInProgress(this, table_name, deadline, alter_in_progress);
+ TableIdentifierPB table;
+ table.set_table_name(table_name);
+ return data_->IsAlterTableInProgress(this, std::move(table), deadline,
+ alter_in_progress);
}
Status KuduClient::GetTableSchema(const string& table_name,
@@ -706,6 +713,7 @@ Status KuduTableCreator::Create() {
// Build request.
CreateTableRequestPB req;
+ CreateTableResponsePB resp;
req.set_name(data_->table_name_);
if (data_->num_replicas_ != boost::none) {
req.set_num_replicas(data_->num_replicas_.get());
@@ -750,10 +758,9 @@ Status KuduTableCreator::Create() {
} else {
deadline += data_->client_->default_admin_operation_timeout();
}
-
RETURN_NOT_OK_PREPEND(data_->client_->data_->CreateTable(data_->client_,
req,
- *data_->schema_,
+ &resp,
deadline,
!data_->range_partition_bounds_.empty()),
Substitute("Error creating table $0 on the master",
@@ -761,8 +768,10 @@ Status KuduTableCreator::Create() {
// Spin until the table is fully created, if requested.
if (data_->wait_) {
+ TableIdentifierPB table;
+ table.set_table_id(resp.table_id());
RETURN_NOT_OK(data_->client_->data_->WaitForCreateTableToFinish(data_->client_,
- data_->table_name_,
+ table,
deadline));
}
@@ -1109,13 +1118,14 @@ KuduTableAlterer* KuduTableAlterer::wait(bool wait) {
Status KuduTableAlterer::Alter() {
AlterTableRequestPB req;
+ AlterTableResponsePB resp;
RETURN_NOT_OK(data_->ToRequest(&req));
MonoDelta timeout = data_->timeout_.Initialized() ?
data_->timeout_ :
data_->client_->default_admin_operation_timeout();
MonoTime deadline = MonoTime::Now() + timeout;
- RETURN_NOT_OK(data_->client_->data_->AlterTable(data_->client_, req, deadline,
+ RETURN_NOT_OK(data_->client_->data_->AlterTable(data_->client_, req, &resp, deadline,
data_->has_alter_partitioning_steps));
if (data_->has_alter_partitioning_steps) {
@@ -1139,9 +1149,15 @@ Status KuduTableAlterer::Alter() {
}
if (data_->wait_) {
- string alter_name = data_->rename_to_.get_value_or(data_->table_name_);
+ if (!resp.has_table_id()) {
+ return Status::NotSupported("Alter Table succeeded but the server's "
+ "response did not include a table ID. This server is too old to wait "
+ "for alter table to finish");
+ }
+ TableIdentifierPB table;
+ table.set_table_id(resp.table_id());
RETURN_NOT_OK(data_->client_->data_->WaitForAlterTableToFinish(
- data_->client_, alter_name, deadline));
+ data_->client_, table, deadline));
}
return Status::OK();
http://git-wip-us.apache.org/repos/asf/kudu/blob/e171466e/src/kudu/client/client.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/client.h b/src/kudu/client/client.h
index 29456e0..cc063fd 100644
--- a/src/kudu/client/client.h
+++ b/src/kudu/client/client.h
@@ -311,7 +311,7 @@ class KUDU_EXPORT KuduClient : public sp::enable_shared_from_this<KuduClient> {
/// the operation is in progress.
/// @return Operation status.
Status IsCreateTableInProgress(const std::string& table_name,
- bool *create_in_progress);
+ bool* create_in_progress);
/// Delete/drop a table.
///
@@ -337,7 +337,7 @@ class KUDU_EXPORT KuduClient : public sp::enable_shared_from_this<KuduClient> {
/// the operation is in progress.
/// @return Operation status.
Status IsAlterTableInProgress(const std::string& table_name,
- bool *alter_in_progress);
+ bool* alter_in_progress);
/// Get table's schema.
///
/// @param [in] table_name
http://git-wip-us.apache.org/repos/asf/kudu/blob/e171466e/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 95d71b7..bc2a501 100644
--- a/src/kudu/integration-tests/alter_table-test.cc
+++ b/src/kudu/integration-tests/alter_table-test.cc
@@ -23,6 +23,7 @@
#include <memory>
#include <ostream>
#include <string>
+#include <thread>
#include <utility>
#include <vector>
@@ -64,6 +65,7 @@
#include "kudu/tserver/ts_tablet_manager.h"
#include "kudu/util/monotime.h"
#include "kudu/util/random.h"
+#include "kudu/util/scoped_cleanup.h"
#include "kudu/util/status.h"
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"
@@ -2066,4 +2068,38 @@ TEST_F(ReplicatedAlterTableTest, TestReplicatedAlter) {
});
}
+TEST_F(AlterTableTest, TestRenameStillCreatingTable) {
+ const string kNewTableName = "foo";
+
+ // Start creating the table in a separate thread.
+ std::thread creator_thread([&](){
+ unique_ptr<KuduTableCreator> creator(client_->NewTableCreator());
+ CHECK_OK(creator->table_name(kNewTableName)
+ .schema(&schema_)
+ .set_range_partition_columns({ "c0" })
+ .num_replicas(1)
+ .Create());
+ });
+ auto cleanup = MakeScopedCleanup([&]() {
+ creator_thread.join();
+ });
+
+ // While the table is being created, change its name. We may have to retry a
+ // few times as this races with table creation.
+ //
+ // If the logic that waits for table creation to finish finds the table by
+ // name, this rename would cause it to fail and the test would crash.
+ while (true) {
+ unique_ptr<KuduTableAlterer> alterer(client_->NewTableAlterer(kNewTableName));
+ Status s = alterer->RenameTo("foo2")->Alter();
+ if (s.ok()) {
+ break;
+ }
+ if (!s.IsNotFound()) {
+ SCOPED_TRACE(s.ToString());
+ FAIL();
+ }
+ }
+}
+
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/kudu/blob/e171466e/src/kudu/integration-tests/master_failover-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/master_failover-itest.cc b/src/kudu/integration-tests/master_failover-itest.cc
index 93106b7..6780449 100644
--- a/src/kudu/integration-tests/master_failover-itest.cc
+++ b/src/kudu/integration-tests/master_failover-itest.cc
@@ -25,7 +25,6 @@
#include <glog/logging.h>
#include <gtest/gtest.h>
-#include "kudu/client/client-internal.h"
#include "kudu/client/client-test-util.h"
#include "kudu/client/client.h"
#include "kudu/client/schema.h"
@@ -202,9 +201,13 @@ TEST_F(MasterFailoverTest, TestPauseAfterCreateTableIssued) {
ASSERT_OK(cluster_->master(leader_idx)->Pause());
ScopedResumeExternalDaemon resume_daemon(cluster_->master(leader_idx));
- MonoTime deadline = MonoTime::Now() + MonoDelta::FromSeconds(90);
- ASSERT_OK(client_->data_->WaitForCreateTableToFinish(client_.get(),
- kTableName, deadline));
+ AssertEventually([&]() {
+ bool in_progress;
+ ASSERT_OK(client_->IsCreateTableInProgress(kTableName, &in_progress));
+ ASSERT_FALSE(in_progress);
+ }, MonoDelta::FromSeconds(90));
+ NO_PENDING_FATALS();
+
shared_ptr<KuduTable> table;
ASSERT_OK(client_->OpenTable(kTableName, &table));
ASSERT_EQ(0, CountTableRows(table.get()));
@@ -310,20 +313,12 @@ TEST_F(MasterFailoverTest, TestKUDU1374) {
// 5. Leader master sees that all tablets in the table now have the new
// schema and ends the AlterTable() operation.
// 6. The next IsAlterTableInProgress() call returns false.
- MonoTime now = MonoTime::Now();
- MonoTime deadline = now + MonoDelta::FromSeconds(90);
- while (now < deadline) {
+ AssertEventually([&]() {
bool in_progress;
ASSERT_OK(client_->IsAlterTableInProgress(kTableName, &in_progress));
- if (!in_progress) {
- break;
- }
-
- SleepFor(MonoDelta::FromMilliseconds(100));
- now = MonoTime::Now();
- }
- ASSERT_TRUE(now < deadline)
- << "Deadline elapsed before alter table completed";
+ ASSERT_FALSE(in_progress);
+ }, MonoDelta::FromSeconds(90));
+ NO_PENDING_FATALS();
}
TEST_F(MasterFailoverTest, TestMasterUUIDResolution) {