This is an automated email from the ASF dual-hosted git repository.
granthenke 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 a701817 KUDU-3164: Add table comment support to Kudu
a701817 is described below
commit a7018179125c49f519600c7f8b284e8bd83e0a38
Author: Grant Henke <[email protected]>
AuthorDate: Tue May 4 16:50:13 2021 -0500
KUDU-3164: Add table comment support to Kudu
This patch adds the basic plumbing for table comments,
synchronizing comments with HMS both using the
notification log listener and via tooling, and setting the
comment on CREATE TABLE and ALTER TABLE requests in
the C++ client.
The maximum comment length is 256 characters by default
which aligns with HMS/Apache Impala maximum comment
lengths, but it's configurable with the
max_table_comment_length flag.
Supporting this in the Java and Python clients, and backup
and restore jobs will come in follow on patches.
Change-Id: Ic4dbf4018c38fdd0c7f748f4dfc26816f22bd5b2
Reviewed-on: http://gerrit.cloudera.org:8080/17399
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <[email protected]>
---
src/kudu/client/client-internal.cc | 8 +-
src/kudu/client/client-internal.h | 2 +
src/kudu/client/client-test.cc | 111 ++++++++++++++++++-----
src/kudu/client/client.cc | 21 ++++-
src/kudu/client/client.h | 18 ++++
src/kudu/client/client.proto | 2 +
src/kudu/client/scan_token-internal.cc | 5 +-
src/kudu/client/table-internal.cc | 2 +
src/kudu/client/table-internal.h | 2 +
src/kudu/client/table_alterer-internal.cc | 4 +
src/kudu/client/table_alterer-internal.h | 1 +
src/kudu/client/table_creator-internal.h | 2 +
src/kudu/hms/hms_catalog-test.cc | 56 +++++++-----
src/kudu/hms/hms_catalog.cc | 17 ++--
src/kudu/hms/hms_catalog.h | 6 +-
src/kudu/hms/hms_client.cc | 1 +
src/kudu/hms/hms_client.h | 1 +
src/kudu/integration-tests/hms_itest-base.cc | 10 ++
src/kudu/integration-tests/hms_itest-base.h | 5 +
src/kudu/integration-tests/master_hms-itest.cc | 27 ++++++
src/kudu/master/catalog_manager.cc | 87 ++++++++++++------
src/kudu/master/catalog_manager.h | 6 ++
src/kudu/master/hms_notification_log_listener.cc | 12 ++-
src/kudu/master/master-test.cc | 22 ++++-
src/kudu/master/master.proto | 11 +++
src/kudu/master/master_path_handlers.cc | 2 +
src/kudu/tools/kudu-admin-test.cc | 7 +-
src/kudu/tools/kudu-tool-test.cc | 62 ++++++++-----
src/kudu/tools/tool_action_hms.cc | 47 ++++++++--
src/kudu/tools/tool_action_table.cc | 3 +
www/table.mustache | 1 +
31 files changed, 442 insertions(+), 119 deletions(-)
diff --git a/src/kudu/client/client-internal.cc
b/src/kudu/client/client-internal.cc
index 30d76a9..95624d2 100644
--- a/src/kudu/client/client-internal.cc
+++ b/src/kudu/client/client-internal.cc
@@ -517,6 +517,7 @@ Status KuduClient::Data::OpenTable(KuduClient* client,
string table_name;
int num_replicas;
string owner;
+ string comment;
PartitionSchema partition_schema;
map<string, string> extra_configs;
MonoTime deadline = MonoTime::Now() + default_admin_operation_timeout_;
@@ -529,6 +530,7 @@ Status KuduClient::Data::OpenTable(KuduClient* client,
&table_name,
&num_replicas,
&owner,
+ &comment,
&extra_configs));
// When the table name is specified, use the caller-provided table name.
@@ -542,7 +544,7 @@ Status KuduClient::Data::OpenTable(KuduClient* client,
// map to reuse KuduTable instances.
table->reset(new KuduTable(client->shared_from_this(),
effective_table_name, table_id, num_replicas,
- owner, schema, partition_schema, extra_configs));
+ owner, comment, schema, partition_schema,
extra_configs));
// When opening a table, clear the existing cached non-covered range entries.
// This avoids surprises where a new table instance won't be able to see the
@@ -561,6 +563,7 @@ Status KuduClient::Data::GetTableSchema(KuduClient* client,
std::string* table_name,
int* num_replicas,
std::string* owner,
+ std::string* comment,
map<string, string>* extra_configs) {
GetTableSchemaRequestPB req;
GetTableSchemaResponsePB resp;
@@ -604,6 +607,9 @@ Status KuduClient::Data::GetTableSchema(KuduClient* client,
if (owner) {
*owner = resp.owner();
}
+ if (comment) {
+ *comment = resp.comment();
+ }
if (extra_configs) {
map<string, string> result(resp.extra_configs().begin(),
resp.extra_configs().end());
*extra_configs = std::move(result);
diff --git a/src/kudu/client/client-internal.h
b/src/kudu/client/client-internal.h
index 4f8153a..2971f51 100644
--- a/src/kudu/client/client-internal.h
+++ b/src/kudu/client/client-internal.h
@@ -149,6 +149,7 @@ class KuduClient::Data {
// 'table_name' The table unique name.
// 'num_replicas' The table replication factor.
// 'owner' The owner of the table.
+ // 'comment' The comment for the table.
// 'extra_configs' The table's extra configuration properties.
Status GetTableSchema(KuduClient* client,
const MonoTime& deadline,
@@ -159,6 +160,7 @@ class KuduClient::Data {
std::string* table_name,
int* num_replicas,
std::string* owner,
+ std::string* comment,
std::map<std::string, std::string>* extra_configs);
Status InitLocalHostNames();
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index 990e3e2..380bc3a 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -158,6 +158,7 @@ DECLARE_int32(log_inject_latency_ms_stddev);
DECLARE_int32(master_inject_latency_on_tablet_lookups_ms);
DECLARE_int32(max_column_comment_length);
DECLARE_int32(max_create_tablets_per_ts);
+DECLARE_int32(max_table_comment_length);
DECLARE_int32(raft_heartbeat_interval_ms);
DECLARE_int32(scanner_batch_size_rows);
DECLARE_int32(scanner_gc_check_interval_us);
@@ -5815,26 +5816,30 @@ TEST_F(ClientTest, TestReadAtSnapshotNoTimestampSet) {
TEST_F(ClientTest, TestCreateTableWithValidComment) {
const string kTableName = "table_comment";
- const string kLongComment(FLAGS_max_column_comment_length, 'x');
+ const string kLongColumnComment(FLAGS_max_column_comment_length, 'x');
+ const string kLongTableComment(FLAGS_max_table_comment_length, 'x');
- // Create a table with comment.
+ // Create a table with a table and column comment.
{
KuduSchema schema;
KuduSchemaBuilder schema_builder;
schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
-
schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32)->Comment(kLongComment);
+
schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32)->Comment(kLongColumnComment);
ASSERT_OK(schema_builder.Build(&schema));
unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
ASSERT_OK(table_creator->table_name(kTableName)
- .schema(&schema).num_replicas(1).set_range_partition_columns({ "key"
}).Create());
+ .schema(&schema).num_replicas(1).set_range_partition_columns({ "key" })
+ .set_comment(kLongTableComment)
+ .Create());
}
- // Open the table and verify the comment.
+ // Open the table and verify the comments.
{
- KuduSchema schema;
- ASSERT_OK(client_->GetTableSchema(kTableName, &schema));
- ASSERT_EQ(schema.Column(1).name(), "val");
- ASSERT_EQ(schema.Column(1).comment(), kLongComment);
+ shared_ptr<KuduTable> table;
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
+ ASSERT_EQ(table->comment(), kLongTableComment);
+ ASSERT_EQ(table->schema().Column(1).name(), "val");
+ ASSERT_EQ(table->schema().Column(1).comment(), kLongColumnComment);
}
}
@@ -5842,31 +5847,36 @@ TEST_F(ClientTest, TestAlterTableWithValidComment) {
const string kTableName = "table_comment";
const auto AlterAndVerify = [&] (int i) {
{
- // The comment length should be less and equal than
FLAGS_max_column_comment_length.
+ // The comment length should be less and equal than
FLAGS_max_column_comment_length
+ // and FLAGS_max_table_comment_length.
string kLongComment(i * 16, 'x');
- // Alter the table with comment.
+ // Alter the table with a table and column comment.
unique_ptr<KuduTableAlterer>
table_alterer(client_->NewTableAlterer(kTableName));
+ table_alterer->SetComment(kLongComment);
table_alterer->AlterColumn("val")->Comment(kLongComment);
ASSERT_OK(table_alterer->Alter());
- // Open the table and verify the comment.
- KuduSchema schema;
- ASSERT_OK(client_->GetTableSchema(kTableName, &schema));
- ASSERT_EQ(schema.Column(1).name(), "val");
- ASSERT_EQ(schema.Column(1).comment(), kLongComment);
+ // Open the table and verify the comments.
+ shared_ptr<KuduTable> table;
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
+ ASSERT_EQ(table->comment(), kLongComment);
+ ASSERT_EQ(table->schema().Column(1).name(), "val");
+ ASSERT_EQ(table->schema().Column(1).comment(), kLongComment);
}
{
// Delete the comment.
unique_ptr<KuduTableAlterer>
table_alterer(client_->NewTableAlterer(kTableName));
+ table_alterer->SetComment("");
table_alterer->AlterColumn("val")->Comment("");
ASSERT_OK(table_alterer->Alter());
- // Open the table and verify the comment.
- KuduSchema schema;
- ASSERT_OK(client_->GetTableSchema(kTableName, &schema));
- ASSERT_EQ(schema.Column(1).name(), "val");
- ASSERT_EQ(schema.Column(1).comment(), "");
+ // Open the table and verify the comments.
+ shared_ptr<KuduTable> table;
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
+ ASSERT_EQ(table->comment(), "");
+ ASSERT_EQ(table->schema().Column(1).name(), "val");
+ ASSERT_EQ(table->schema().Column(1).comment(), "");
}
};
@@ -5887,7 +5897,7 @@ TEST_F(ClientTest, TestAlterTableWithValidComment) {
}
}
-TEST_F(ClientTest, TestCreateAndAlterTableWithInvalidComment) {
+TEST_F(ClientTest, TestCreateAndAlterTableWithInvalidColumnComment) {
const string kTableName = "table_comment";
const vector<pair<string, string>> kCases = {
{string(FLAGS_max_column_comment_length + 1, 'x'), "longer than maximum
permitted length"},
@@ -5895,7 +5905,7 @@ TEST_F(ClientTest,
TestCreateAndAlterTableWithInvalidComment) {
{string("foo\xf0\x28\x8c\xbc", 7), "invalid column comment: invalid UTF8
sequence"}
};
- // Create tables with invalid comment.
+ // Create tables with invalid column comment.
for (const auto& test_case : kCases) {
const auto& comment = test_case.first;
const auto& substr = test_case.second;
@@ -5924,7 +5934,7 @@ TEST_F(ClientTest,
TestCreateAndAlterTableWithInvalidComment) {
.schema(&schema).num_replicas(1).set_range_partition_columns({ "key"
}).Create());
}
- // Alter tables with invalid comment.
+ // Alter tables with invalid column comment.
for (const auto& test_case : kCases) {
const auto& comment = test_case.first;
const auto& substr = test_case.second;
@@ -5938,6 +5948,59 @@ TEST_F(ClientTest,
TestCreateAndAlterTableWithInvalidComment) {
}
}
+TEST_F(ClientTest, TestCreateAndAlterTableWithInvalidTableComment) {
+ const string kTableName = "table_comment";
+ const vector<pair<string, string>> kCases = {
+ {string(FLAGS_max_table_comment_length + 1, 'x'), "longer than maximum
permitted length"},
+ {string("foo\0bar", 7), "invalid table comment: identifier must not
contain null bytes"},
+ {string("foo\xf0\x28\x8c\xbc", 7), "invalid table comment: invalid UTF8
sequence"}
+ };
+
+ // Create tables with invalid table comment.
+ for (const auto& test_case : kCases) {
+ const auto& comment = test_case.first;
+ const auto& substr = test_case.second;
+ SCOPED_TRACE(comment);
+
+ KuduSchema schema;
+ KuduSchemaBuilder schema_builder;
+
schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
+ schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32);
+ ASSERT_OK(schema_builder.Build(&schema));
+ unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+ Status s = table_creator->table_name(kTableName)
+ .schema(&schema).num_replicas(1).set_range_partition_columns({ "key" })
+ .set_comment(comment)
+ .Create();
+ ASSERT_STR_CONTAINS(s.ToString(), substr);
+ }
+
+ // Create a table for later alterer.
+ {
+ KuduSchema schema;
+ KuduSchemaBuilder schema_builder;
+
schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey();
+ schema_builder.AddColumn("val")->Type(KuduColumnSchema::INT32);
+ ASSERT_OK(schema_builder.Build(&schema));
+ unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+ ASSERT_OK(table_creator->table_name(kTableName)
+ .schema(&schema).num_replicas(1).set_range_partition_columns({ "key"
}).Create());
+ }
+
+ // Alter tables with invalid table comment.
+ for (const auto& test_case : kCases) {
+ const auto& comment = test_case.first;
+ const auto& substr = test_case.second;
+ SCOPED_TRACE(comment);
+
+ // Alter the table.
+ unique_ptr<KuduTableAlterer>
table_alterer(client_->NewTableAlterer(kTableName));
+ table_alterer->SetComment(comment);
+ Status s = table_alterer->Alter();
+ ASSERT_STR_CONTAINS(s.ToString(), substr);
+ }
+}
+
TEST_F(ClientTest, TestAlterTableChangeOwner) {
const string kTableName = "table_owner";
const string kOriginalOwner = "alice";
diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc
index dcae85c..fc91ec2 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -529,6 +529,7 @@ Status KuduClient::GetTableSchema(const string& table_name,
nullptr, // table name
nullptr, // number of replicas
nullptr, // owner
+ nullptr, // comment
nullptr); // extra configs
}
@@ -842,6 +843,11 @@ KuduTableCreator& KuduTableCreator::set_owner(const
string& owner) {
return *this;
}
+KuduTableCreator& KuduTableCreator::set_comment(const string& comment) {
+ data_->comment_ = comment;
+ return *this;
+}
+
KuduTableCreator& KuduTableCreator::split_rows(const vector<const
KuduPartialRow*>& rows) {
for (const KuduPartialRow* row : rows) {
data_->range_partition_splits_.emplace_back(const_cast<KuduPartialRow*>(row));
@@ -918,6 +924,9 @@ Status KuduTableCreator::Create() {
if (data_->owner_ != boost::none) {
req.set_owner(data_->owner_.get());
}
+ if (data_->comment_ != boost::none) {
+ req.set_comment(data_->comment_.get());
+ }
RETURN_NOT_OK_PREPEND(SchemaToPB(*data_->schema_->schema_,
req.mutable_schema(),
SCHEMA_PB_WITHOUT_WRITE_DEFAULT),
"Invalid schema");
@@ -1022,10 +1031,11 @@ KuduTable::KuduTable(const shared_ptr<KuduClient>&
client,
const string& id,
int num_replicas,
const string& owner,
+ const string& comment,
const KuduSchema& schema,
const PartitionSchema& partition_schema,
const map<string, string>& extra_configs)
- : data_(new KuduTable::Data(client, name, id, num_replicas, owner,
+ : data_(new KuduTable::Data(client, name, id, num_replicas, owner, comment,
schema, partition_schema, extra_configs)) {
}
@@ -1045,6 +1055,10 @@ const KuduSchema& KuduTable::schema() const {
return data_->schema_;
}
+const string& KuduTable::comment() const {
+ return data_->comment_;
+}
+
int KuduTable::num_replicas() const {
return data_->num_replicas_;
}
@@ -1381,6 +1395,11 @@ KuduTableAlterer* KuduTableAlterer::SetOwner(const
string& new_owner) {
return this;
}
+KuduTableAlterer* KuduTableAlterer::SetComment(const string& new_comment) {
+ data_->set_comment_to_ = new_comment;
+ return this;
+}
+
KuduColumnSpec* KuduTableAlterer::AddColumn(const string& name) {
Data::Step s = { AlterTableRequestPB::ADD_COLUMN,
new KuduColumnSpec(name), nullptr, nullptr };
diff --git a/src/kudu/client/client.h b/src/kudu/client/client.h
index 59aee75..8742738 100644
--- a/src/kudu/client/client.h
+++ b/src/kudu/client/client.h
@@ -1207,6 +1207,13 @@ class KUDU_EXPORT KuduTableCreator {
/// @return Reference to the modified table creator.
KuduTableCreator& set_owner(const std::string& owner);
+ /// Set the table comment.
+ ///
+ /// @param [in] comment
+ /// The comment on the table.
+ /// @return Reference to the modified table creator.
+ KuduTableCreator& set_comment(const std::string& comment);
+
/// @deprecated Use @c add_range_partition_split() instead.
///
/// @param [in] split_rows
@@ -1386,6 +1393,9 @@ class KUDU_EXPORT KuduTable : public
sp::enable_shared_from_this<KuduTable> {
/// @return Reference to the table's schema object.
const KuduSchema& schema() const;
+ /// @return Comment string for the table.
+ const std::string& comment() const;
+
/// @return Replication factor of the table.
int num_replicas() const;
@@ -1619,6 +1629,7 @@ class KUDU_EXPORT KuduTable : public
sp::enable_shared_from_this<KuduTable> {
const std::string& id,
int num_replicas,
const std::string& owner,
+ const std::string& comment,
const KuduSchema& schema,
const PartitionSchema& partition_schema,
const std::map<std::string, std::string>& extra_configs);
@@ -1658,6 +1669,13 @@ class KUDU_EXPORT KuduTableAlterer {
/// @return Raw pointer to this alterer object.
KuduTableAlterer* SetOwner(const std::string& new_owner);
+ /// Set the comment on the table.
+ ///
+ /// @param [in] new_comment
+ /// The new comment on the table.
+ /// @return Raw pointer to this alterer object.
+ KuduTableAlterer* SetComment(const std::string& new_comment);
+
/// Add a new column to the table.
///
/// When adding a column, you must specify the default value of the new
diff --git a/src/kudu/client/client.proto b/src/kudu/client/client.proto
index be4c106..b56465d 100644
--- a/src/kudu/client/client.proto
+++ b/src/kudu/client/client.proto
@@ -41,6 +41,8 @@ message TableMetadataPB {
map<string, string> extra_configs = 6;
optional string owner = 7;
+
+ optional string comment = 8;
}
// Metdata about a single server.
diff --git a/src/kudu/client/scan_token-internal.cc
b/src/kudu/client/scan_token-internal.cc
index 00afa2b..117b70d 100644
--- a/src/kudu/client/scan_token-internal.cc
+++ b/src/kudu/client/scan_token-internal.cc
@@ -135,8 +135,8 @@ Status KuduScanToken::Data::PBIntoScanner(KuduClient*
client,
map<string, string> extra_configs(metadata.extra_configs().begin(),
metadata.extra_configs().end());
table.reset(new KuduTable(client->shared_from_this(),
metadata.table_name(),
- metadata.table_id(), metadata.num_replicas(), metadata.owner(),
kudu_schema,
- partition_schema, extra_configs));
+ metadata.table_id(), metadata.num_replicas(), metadata.owner(),
metadata.comment(),
+ kudu_schema, partition_schema, extra_configs));
} else {
TableIdentifierPB table_identifier;
if (message.has_table_id()) {
@@ -337,6 +337,7 @@ Status
KuduScanTokenBuilder::Data::Build(vector<KuduScanToken*>* tokens) {
table_pb.set_table_name(table->name());
table_pb.set_num_replicas(table->num_replicas());
table_pb.set_owner(table->owner());
+ table_pb.set_comment(table->comment());
SchemaPB schema_pb;
RETURN_NOT_OK(SchemaToPB(KuduSchema::ToSchema(table->schema()),
&schema_pb));
*table_pb.mutable_schema() = std::move(schema_pb);
diff --git a/src/kudu/client/table-internal.cc
b/src/kudu/client/table-internal.cc
index f3e8ad2..a1572c7 100644
--- a/src/kudu/client/table-internal.cc
+++ b/src/kudu/client/table-internal.cc
@@ -34,6 +34,7 @@ KuduTable::Data::Data(shared_ptr<KuduClient> client,
string id,
int num_replicas,
string owner,
+ string comment,
const KuduSchema& schema,
PartitionSchema partition_schema,
map<string, string> extra_configs)
@@ -42,6 +43,7 @@ KuduTable::Data::Data(shared_ptr<KuduClient> client,
id_(std::move(id)),
num_replicas_(num_replicas),
owner_(std::move(owner)),
+ comment_(std::move(comment)),
schema_(schema),
partition_schema_(std::move(partition_schema)),
extra_configs_(std::move(extra_configs)) {
diff --git a/src/kudu/client/table-internal.h b/src/kudu/client/table-internal.h
index 5c894d6..d6a632b 100644
--- a/src/kudu/client/table-internal.h
+++ b/src/kudu/client/table-internal.h
@@ -44,6 +44,7 @@ class KuduTable::Data {
std::string id,
int num_replicas,
std::string owner,
+ std::string comment,
const KuduSchema& schema,
PartitionSchema partition_schema,
std::map<std::string, std::string> extra_configs);
@@ -71,6 +72,7 @@ class KuduTable::Data {
const std::string id_;
const int num_replicas_;
const std::string owner_;
+ const std::string comment_;
// TODO(unknown): figure out how we deal with a schema change from the client
// perspective. Do we make them call a RefreshSchema() method? Or maybe
diff --git a/src/kudu/client/table_alterer-internal.cc
b/src/kudu/client/table_alterer-internal.cc
index 874d02b..2d0254d 100644
--- a/src/kudu/client/table_alterer-internal.cc
+++ b/src/kudu/client/table_alterer-internal.cc
@@ -60,6 +60,7 @@ Status KuduTableAlterer::Data::ToRequest(AlterTableRequestPB*
req) {
if (!rename_to_.is_initialized() &&
!new_extra_configs_ &&
!set_owner_to_.is_initialized() &&
+ !set_comment_to_.is_initialized() &&
!disk_size_limit_ &&
!row_count_limit_ &&
steps_.empty()) {
@@ -79,6 +80,9 @@ Status KuduTableAlterer::Data::ToRequest(AlterTableRequestPB*
req) {
if (set_owner_to_.is_initialized()) {
req->set_new_table_owner(set_owner_to_.get());
}
+ if (set_comment_to_.is_initialized()) {
+ req->set_new_table_comment(set_comment_to_.get());
+ }
if (schema_ != nullptr) {
RETURN_NOT_OK(SchemaToPB(*schema_, req->mutable_schema(),
diff --git a/src/kudu/client/table_alterer-internal.h
b/src/kudu/client/table_alterer-internal.h
index 641daea..9959aee 100644
--- a/src/kudu/client/table_alterer-internal.h
+++ b/src/kudu/client/table_alterer-internal.h
@@ -76,6 +76,7 @@ class KuduTableAlterer::Data {
boost::optional<std::string> rename_to_;
boost::optional<std::string> set_owner_to_;
+ boost::optional<std::string> set_comment_to_;
boost::optional<std::map<std::string, std::string>> new_extra_configs_;
diff --git a/src/kudu/client/table_creator-internal.h
b/src/kudu/client/table_creator-internal.h
index 36225ca..ec75a29 100644
--- a/src/kudu/client/table_creator-internal.h
+++ b/src/kudu/client/table_creator-internal.h
@@ -62,6 +62,8 @@ class KuduTableCreator::Data {
boost::optional<std::string> owner_;
+ boost::optional<std::string> comment_;
+
boost::optional<int> num_replicas_;
boost::optional<std::string> dimension_label_;
diff --git a/src/kudu/hms/hms_catalog-test.cc b/src/kudu/hms/hms_catalog-test.cc
index 974876b..58ef042 100644
--- a/src/kudu/hms/hms_catalog-test.cc
+++ b/src/kudu/hms/hms_catalog-test.cc
@@ -219,7 +219,8 @@ class HmsCatalogTest : public KuduTest {
const string& table_id,
const string& cluster_id,
const boost::optional<const string&>& owner,
- const Schema& schema) {
+ const Schema& schema,
+ const string& comment) {
hive::Table table;
ASSERT_OK(hms_client_->GetTable(database_name, table_name, &table));
@@ -232,6 +233,7 @@ class HmsCatalogTest : public KuduTest {
EXPECT_EQ(table.parameters[HmsClient::kKuduClusterIdKey], cluster_id);
EXPECT_EQ(table.parameters[HmsClient::kKuduMasterAddrsKey], kMasterAddrs);
EXPECT_EQ(table.parameters[HmsClient::kStorageHandlerKey],
HmsClient::kKuduStorageHandler);
+ EXPECT_EQ(table.parameters[HmsClient::kTableCommentKey], comment);
for (int column_idx = 0; column_idx < schema.num_columns(); column_idx++) {
EXPECT_EQ(table.sd.cols[column_idx].name,
schema.columns()[column_idx].name());
@@ -304,30 +306,32 @@ TEST_P(HmsCatalogTestParameterized, TestTableLifecycle) {
const string kHmsAlteredTableName = "altered_table_name";
const string kAlteredTableName = Substitute("$0.$1", kHmsDatabase,
kHmsAlteredTableName);
const string kOwner = "alice";
+ const string kComment = "comment";
Schema schema = AllTypesSchema();
// Create the table.
ASSERT_OK(hms_catalog_->CreateTable(kTableId, kTableName,
- kClusterId, kOwner, schema));
+ kClusterId, kOwner, schema, kComment));
NO_FATALS(CheckTable(kHmsDatabase, kHmsTableName, kTableId,
- kClusterId, kOwner, schema));
+ kClusterId, kOwner, schema, kComment));
// Create the table again, and check that the expected failure occurs.
Status s = hms_catalog_->CreateTable(kTableId, kTableName,
- kClusterId, kOwner, schema);
+ kClusterId, kOwner, schema, kComment);
ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
- NO_FATALS(CheckTable(kHmsDatabase, kHmsTableName, kTableId, kClusterId,
kOwner, schema));
+ NO_FATALS(CheckTable(kHmsDatabase, kHmsTableName, kTableId, kClusterId,
kOwner,
+ schema, kComment));
// Alter the table.
SchemaBuilder b(schema);
b.AddColumn("new_column", DataType::INT32);
Schema altered_schema = b.Build();
ASSERT_OK(hms_catalog_->AlterTable(kTableId, kTableName, kAlteredTableName,
- kClusterId, kOwner, altered_schema));
+ kClusterId, kOwner, altered_schema, kComment));
NO_FATALS(CheckTableDoesNotExist(kHmsDatabase, kHmsTableName));
NO_FATALS(CheckTable(kHmsDatabase, kHmsAlteredTableName, kTableId,
kClusterId, kOwner,
- altered_schema));
+ altered_schema, kComment));
// Drop the table.
ASSERT_OK(hms_catalog_->DropTable(kTableId, kAlteredTableName));
@@ -340,6 +344,7 @@ TEST_P(HmsCatalogTestParameterized, TestTableLifecycle) {
TEST_F(HmsCatalogTest, TestExternalTable) {
const string kTableId = "table-id";
const string kClusterId = "cluster-id";
+ const string kComment = "comment";
// Create the external table (default.ext).
hive::Table external_table;
@@ -360,29 +365,29 @@ TEST_F(HmsCatalogTest, TestExternalTable) {
// Create the Kudu table (default.a).
Schema schema = AllTypesSchema();
ASSERT_OK(hms_catalog_->CreateTable(kTableId, "default.a",
- kClusterId, boost::none, schema));
- NO_FATALS(CheckTable("default", "a", kTableId, kClusterId, boost::none,
schema));
+ kClusterId, boost::none, schema, kComment));
+ NO_FATALS(CheckTable("default", "a", kTableId, kClusterId, boost::none,
schema, kComment));
// Try and create a Kudu table with the same name as the external table.
Status s = hms_catalog_->CreateTable(kTableId, "default.ext",
- kClusterId, boost::none, schema);
+ kClusterId, boost::none, schema, kComment);
EXPECT_TRUE(s.IsAlreadyPresent()) << s.ToString();
NO_FATALS(CheckExternalTable());
// Try and rename the Kudu table to the external table name.
s = hms_catalog_->AlterTable(kTableId, "default.a", "default.ext",
- kClusterId, boost::none, schema);
+ kClusterId, boost::none, schema, kComment);
EXPECT_TRUE(s.IsIllegalState()) << s.ToString();
NO_FATALS(CheckExternalTable());
- NO_FATALS(CheckTable("default", "a", kTableId, kClusterId, boost::none,
schema));
+ NO_FATALS(CheckTable("default", "a", kTableId, kClusterId, boost::none,
schema, kComment));
// Try and rename the external table. This shouldn't succeed because the
Table
// ID doesn't match.
s = hms_catalog_->AlterTable(kTableId, "default.ext", "default.b",
- kClusterId, boost::none, schema);
+ kClusterId, boost::none, schema, kComment);
EXPECT_TRUE(s.IsNotFound()) << s.ToString();
NO_FATALS(CheckExternalTable());
- NO_FATALS(CheckTable("default", "a", kTableId, kClusterId, boost::none,
schema));
+ NO_FATALS(CheckTable("default", "a", kTableId, kClusterId, boost::none,
schema, kComment));
NO_FATALS(CheckTableDoesNotExist("default", "b"));
// Try and drop the external table as if it were a Kudu table.
@@ -405,6 +410,7 @@ TEST_F(HmsCatalogTest, TestGetKuduTables) {
const string kTableName = "external_table";
const string kNonKuduTableName = "non_kudu_table";
const string kOwner = "alice";
+ const string kComment = "comment";
// Create a legacy Impala managed table, a legacy Impala external table, a
// Kudu table, and a non Kudu table.
@@ -418,7 +424,7 @@ TEST_F(HmsCatalogTest, TestGetKuduTables) {
ASSERT_OK(hms_client_->GetTable("db", "external_table", &table));
ASSERT_OK(hms_catalog_->CreateTable("fake-id", "db.table",
- kClusterId, kOwner, Schema()));
+ kClusterId, kOwner, Schema(), kComment));
hive::Table non_kudu_table;
non_kudu_table.dbName = "db";
@@ -444,10 +450,12 @@ TEST_F(HmsCatalogTest, TestReconnect) {
const string kClusterId = "cluster-id";
const string kHmsDatabase = "default";
const string kOwner = "alice";
+ const string kComment = "comment";
+
Schema schema = AllTypesSchema();
ASSERT_OK(hms_catalog_->CreateTable(kTableId, "default.a",
- kClusterId, kOwner, schema));
- NO_FATALS(CheckTable(kHmsDatabase, "a", kTableId, kClusterId, kOwner,
schema));
+ kClusterId, kOwner, schema, kComment));
+ NO_FATALS(CheckTable(kHmsDatabase, "a", kTableId, kClusterId, kOwner,
schema, kComment));
// Shutdown the HMS and try a few operations.
ASSERT_OK(StopHms());
@@ -456,11 +464,11 @@ TEST_F(HmsCatalogTest, TestReconnect) {
// attempts.
Status s = hms_catalog_->CreateTable(kTableId, "default.b",
- kClusterId, kOwner, schema);
+ kClusterId, kOwner, schema, kComment);
EXPECT_TRUE(s.IsNetworkError()) << s.ToString();
s = hms_catalog_->AlterTable(kTableId, "default.a", "default.c",
- kClusterId, kOwner, schema);
+ kClusterId, kOwner, schema, kComment);
EXPECT_TRUE(s.IsNetworkError()) << s.ToString();
// Start the HMS back up and ensure that the same operations succeed.
@@ -468,15 +476,15 @@ TEST_F(HmsCatalogTest, TestReconnect) {
ASSERT_EVENTUALLY([&] {
// HmsCatalog throttles reconnections, so it's necessary to wait out the
backoff.
ASSERT_OK(hms_catalog_->CreateTable(kTableId, "default.d",
- kClusterId, kOwner, schema));
+ kClusterId, kOwner, schema, kComment));
});
- NO_FATALS(CheckTable(kHmsDatabase, "a", kTableId, kClusterId, kOwner,
schema));
- NO_FATALS(CheckTable(kHmsDatabase, "d", kTableId, kClusterId, kOwner,
schema));
+ NO_FATALS(CheckTable(kHmsDatabase, "a", kTableId, kClusterId, kOwner,
schema, kComment));
+ NO_FATALS(CheckTable(kHmsDatabase, "d", kTableId, kClusterId, kOwner,
schema, kComment));
EXPECT_OK(hms_catalog_->AlterTable(kTableId, "default.a", "default.c",
- kClusterId, kOwner, schema));
- NO_FATALS(CheckTable(kHmsDatabase, "c", kTableId, kClusterId, kOwner,
schema));
+ kClusterId, kOwner, schema, kComment));
+ NO_FATALS(CheckTable(kHmsDatabase, "c", kTableId, kClusterId, kOwner,
schema, kComment));
NO_FATALS(CheckTableDoesNotExist(kHmsDatabase, "a"));
}
diff --git a/src/kudu/hms/hms_catalog.cc b/src/kudu/hms/hms_catalog.cc
index 8eb8b30..a649d53 100644
--- a/src/kudu/hms/hms_catalog.cc
+++ b/src/kudu/hms/hms_catalog.cc
@@ -148,10 +148,11 @@ Status HmsCatalog::CreateTable(const string& id,
const string& cluster_id,
const optional<const string&>& owner,
const Schema& schema,
+ const string& comment,
const string& table_type) {
hive::Table table;
- RETURN_NOT_OK(PopulateTable(id, name, owner, schema, cluster_id,
master_addresses_,
- table_type, &table));
+ RETURN_NOT_OK(PopulateTable(id, name, owner, schema, comment, cluster_id,
+ master_addresses_, table_type, &table));
return ha_client_.Execute([&] (HmsClient* client) {
return client->CreateTable(table, EnvironmentContext());
});
@@ -180,7 +181,8 @@ Status HmsCatalog::UpgradeLegacyImpalaTable(const string&
id,
const string& cluster_id,
const string& db_name,
const string& tb_name,
- const Schema& schema) {
+ const Schema& schema,
+ const string& comment) {
return ha_client_.Execute([&] (HmsClient* client) {
hive::Table table;
RETURN_NOT_OK(client->GetTable(db_name, tb_name, &table));
@@ -199,7 +201,7 @@ Status HmsCatalog::UpgradeLegacyImpalaTable(const string&
id,
table.parameters[HmsClient::kStorageHandlerKey] =
HmsClient::kKuduStorageHandler;
} else {
RETURN_NOT_OK(PopulateTable(id, Substitute("$0.$1", db_name, tb_name),
table.owner, schema,
- cluster_id, master_addresses_,
table.tableType, &table));
+ comment, cluster_id, master_addresses_,
table.tableType, &table));
}
return client->AlterTable(db_name, tb_name, table, EnvironmentContext());
});
@@ -261,6 +263,7 @@ Status HmsCatalog::AlterTable(const string& id,
const string& cluster_id,
optional<const string&> owner,
const Schema& schema,
+ const std::string& comment,
const bool& check_id) {
Slice hms_database;
Slice hms_table;
@@ -294,8 +297,8 @@ Status HmsCatalog::AlterTable(const string& id,
}
// Overwrite fields in the table that have changed, including the new
name.
- RETURN_NOT_OK(PopulateTable(id, new_name, owner, schema, cluster_id,
master_addresses_,
- table.tableType, &table));
+ RETURN_NOT_OK(PopulateTable(id, new_name, owner, schema, comment,
cluster_id,
+ master_addresses_, table.tableType, &table));
return client->AlterTable(hms_database.ToString(), hms_table.ToString(),
table, EnvironmentContext(check_id));
});
@@ -365,6 +368,7 @@ Status HmsCatalog::PopulateTable(const string& id,
const string& name,
const optional<const string&>& owner,
const Schema& schema,
+ const std::string& comment,
const string& cluster_id,
const string& master_addresses,
const string& table_type,
@@ -374,6 +378,7 @@ Status HmsCatalog::PopulateTable(const string& id,
RETURN_NOT_OK(ParseHiveTableIdentifier(name, &hms_database_name,
&hms_table_name));
table->dbName = hms_database_name.ToString();
table->tableName = hms_table_name.ToString();
+ table->parameters[HmsClient::kTableCommentKey] = comment;
if (owner) {
table->owner = *owner;
}
diff --git a/src/kudu/hms/hms_catalog.h b/src/kudu/hms/hms_catalog.h
index f162afe..8285d6a 100644
--- a/src/kudu/hms/hms_catalog.h
+++ b/src/kudu/hms/hms_catalog.h
@@ -75,6 +75,7 @@ class HmsCatalog {
const std::string& cluster_id,
const boost::optional<const std::string&>& owner,
const Schema& schema,
+ const std::string& comment,
const std::string& table_type =
hms::HmsClient::kManagedTable)
WARN_UNUSED_RESULT;
@@ -106,6 +107,7 @@ class HmsCatalog {
const std::string& cluster_id,
boost::optional<const std::string&> owner,
const Schema& schema,
+ const std::string& comment,
const bool& check_id = true) WARN_UNUSED_RESULT;
// Upgrades a legacy Impala table entry in the HMS.
@@ -116,7 +118,8 @@ class HmsCatalog {
const std::string& cluster_id,
const std::string& db_name,
const std::string& tb_name,
- const Schema& schema) WARN_UNUSED_RESULT;
+ const Schema& schema,
+ const std::string& comment)
WARN_UNUSED_RESULT;
// Downgrades to a legacy Impala table entry in the HMS.
//
@@ -166,6 +169,7 @@ class HmsCatalog {
const std::string& name,
const boost::optional<const std::string&>& owner,
const Schema& schema,
+ const std::string& comment,
const std::string& cluster_id,
const std::string& master_addresses,
const std::string& table_type,
diff --git a/src/kudu/hms/hms_client.cc b/src/kudu/hms/hms_client.cc
index 63499d3..8c65bf5 100644
--- a/src/kudu/hms/hms_client.cc
+++ b/src/kudu/hms/hms_client.cc
@@ -137,6 +137,7 @@ const char* const HmsClient::kDbNotificationListener =
const char* const HmsClient::kExternalTableKey = "EXTERNAL";
const char* const HmsClient::kExternalPurgeKey = "external.table.purge";
const char* const HmsClient::kStorageHandlerKey = "storage_handler";
+const char* const HmsClient::kTableCommentKey = "comment";
const char* const HmsClient::kKuduMetastorePlugin =
"org.apache.kudu.hive.metastore.KuduMetastorePlugin";
const char* const HmsClient::kHiveFilterFieldParams =
"hive_filter_field_params__";
diff --git a/src/kudu/hms/hms_client.h b/src/kudu/hms/hms_client.h
index 59aff12..fe39f37 100644
--- a/src/kudu/hms/hms_client.h
+++ b/src/kudu/hms/hms_client.h
@@ -67,6 +67,7 @@ class HmsClient {
static const char* const kKuduMasterEventKey;
static const char* const kKuduCheckIdKey;
static const char* const kStorageHandlerKey;
+ static const char* const kTableCommentKey;
static const char* const kKuduStorageHandler;
static const char* const kHiveFilterFieldParams;
diff --git a/src/kudu/integration-tests/hms_itest-base.cc
b/src/kudu/integration-tests/hms_itest-base.cc
index cc875c5..0ddbf8e 100644
--- a/src/kudu/integration-tests/hms_itest-base.cc
+++ b/src/kudu/integration-tests/hms_itest-base.cc
@@ -160,6 +160,15 @@ Status HmsITestHarness::ChangeHmsOwner(const string&
database_name,
return hms_client_->AlterTable(database_name, table_name, table);
}
+Status HmsITestHarness::ChangeHmsTableComment(const string& database_name,
+ const string& table_name,
+ const string& new_table_comment)
{
+ hive::Table table;
+ RETURN_NOT_OK(hms_client_->GetTable(database_name, table_name, &table));
+ table.parameters[HmsClient::kTableCommentKey] = new_table_comment;
+ return hms_client_->AlterTable(database_name, table_name, table);
+}
+
Status HmsITestHarness::AlterHmsTableDropColumns(const string& database_name,
const string& table_name) {
hive::Table table;
@@ -224,6 +233,7 @@ void HmsITestHarness::CheckTable(const string&
database_name,
ASSERT_EQ(table->client()->cluster_id(),
hms_table.parameters[hms::HmsClient::kKuduClusterIdKey]);
ASSERT_TRUE(iequals(table->name(),
hms_table.parameters[hms::HmsClient::kKuduTableNameKey]));
+ ASSERT_EQ(table->comment(),
hms_table.parameters[hms::HmsClient::kTableCommentKey]);
ASSERT_EQ(HostPort::ToCommaSeparatedString(cluster->master_rpc_addrs()),
hms_table.parameters[hms::HmsClient::kKuduMasterAddrsKey]);
ASSERT_EQ(hms::HmsClient::kKuduStorageHandler,
diff --git a/src/kudu/integration-tests/hms_itest-base.h
b/src/kudu/integration-tests/hms_itest-base.h
index 784f6c8..fe97523 100644
--- a/src/kudu/integration-tests/hms_itest-base.h
+++ b/src/kudu/integration-tests/hms_itest-base.h
@@ -81,6 +81,11 @@ class HmsITestHarness {
const std::string& table_name,
const std::string& new_table_owner) WARN_UNUSED_RESULT;
+ // Changes the table comment in the HMS catalog.
+ Status ChangeHmsTableComment(const std::string& database_name,
+ const std::string& table_name,
+ const std::string& new_table_comment)
WARN_UNUSED_RESULT;
+
// Checks that the Kudu table schema and the HMS table entry in their
// respective catalogs are synchronized for a particular table. It also
// verifies that the table owner is the given user (if not provided,
diff --git a/src/kudu/integration-tests/master_hms-itest.cc
b/src/kudu/integration-tests/master_hms-itest.cc
index b58e8a8..3729235 100644
--- a/src/kudu/integration-tests/master_hms-itest.cc
+++ b/src/kudu/integration-tests/master_hms-itest.cc
@@ -130,6 +130,12 @@ class MasterHmsTest : public ExternalMiniClusterITestBase {
return harness_.ChangeHmsOwner(database_name, table_name, new_table_owner);
}
+ Status ChangeHmsTableComment(const std::string& database_name,
+ const std::string& table_name,
+ const std::string& new_table_comment) {
+ return harness_.ChangeHmsTableComment(database_name, table_name,
new_table_comment);
+ }
+
Status AlterHmsTableDropColumns(const std::string& database_name,
const std::string& table_name) {
return harness_.AlterHmsTableDropColumns(database_name, table_name);
@@ -328,6 +334,27 @@ TEST_F(MasterHmsTest, TestAlterTableOwner) {
});
}
+TEST_F(MasterHmsTest, TestAlterTableComment) {
+ // Create the Kudu table.
+ ASSERT_OK(CreateKuduTable("default", "commentTable"));
+ NO_FATALS(CheckTable("default", "commentTable", /*user=*/ none));
+
+ // Change the table comment through the HMS, and ensure the comment is
handled in Kudu.
+ const string comment_a = "comment a";
+ ASSERT_OK(ChangeHmsTableComment("default", "commentTable", comment_a));
+ ASSERT_EVENTUALLY([&] {
+ NO_FATALS(CheckTable("default", "commentTable", /*user=*/ none));
+ });
+
+ // Change the table comment through Kudu, and ensure the comment is
reflected in HMS.
+ const string comment_b = "comment b";
+ unique_ptr<KuduTableAlterer>
table_alterer(client_->NewTableAlterer("default.commentTable"));
+ ASSERT_OK(table_alterer->SetComment(comment_b)->Alter());
+ ASSERT_EVENTUALLY([&] {
+ NO_FATALS(CheckTable("default", "commentTable", /*user=*/ none));
+ });
+}
+
TEST_F(MasterHmsTest, TestAlterTable) {
// Create the Kudu table.
ASSERT_OK(CreateKuduTable("default", "a"));
diff --git a/src/kudu/master/catalog_manager.cc
b/src/kudu/master/catalog_manager.cc
index c213461..12666d9 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -188,6 +188,9 @@ TAG_FLAG(max_num_columns, unsafe);
DEFINE_int32(max_identifier_length, 256,
"Maximum length of the name of a column or table.");
+DEFINE_int32(max_table_comment_length, 256,
+ "Maximum length of the comment of a table.");
+
DEFINE_int32(max_column_comment_length, 256,
"Maximum length of the comment of a column.");
@@ -1582,13 +1585,22 @@ Status ValidateIdentifier(const string& id) {
return ValidateLengthAndUTF8(id, FLAGS_max_identifier_length);
}
-// Validate a column comment to ensure that it is a valid identifier.
-Status ValidateCommentIdentifier(const string& id) {
- if (id.empty()) {
+// Validate a column comment.
+Status ValidateColumnComment(const string& comment) {
+ if (comment.empty()) {
+ return Status::OK();
+ }
+
+ return ValidateLengthAndUTF8(comment, FLAGS_max_column_comment_length);
+}
+
+// Validate a table comment.
+Status ValidateTableComment(const string& comment) {
+ if (comment.empty()) {
return Status::OK();
}
- return ValidateLengthAndUTF8(id, FLAGS_max_column_comment_length);
+ return ValidateLengthAndUTF8(comment, FLAGS_max_table_comment_length);
}
Status ValidateOwner(const string& name) {
@@ -1602,6 +1614,7 @@ Status ValidateOwner(const string& name) {
// Validate the client-provided schema and name.
Status ValidateClientSchema(const optional<string>& name,
const optional<string>& owner,
+ const optional<string>& comment,
const Schema& schema) {
if (name) {
RETURN_NOT_OK_PREPEND(ValidateIdentifier(name.get()), "invalid table
name");
@@ -1609,10 +1622,13 @@ Status ValidateClientSchema(const optional<string>&
name,
if (owner) {
RETURN_NOT_OK_PREPEND(ValidateOwner(*owner), "invalid owner name");
}
+ if (comment) {
+ RETURN_NOT_OK_PREPEND(ValidateTableComment(*comment), "invalid table
comment");
+ }
for (int i = 0; i < schema.num_columns(); i++) {
RETURN_NOT_OK_PREPEND(ValidateIdentifier(schema.column(i).name()),
"invalid column name");
-
RETURN_NOT_OK_PREPEND(ValidateCommentIdentifier(schema.column(i).comment()),
+ RETURN_NOT_OK_PREPEND(ValidateColumnComment(schema.column(i).comment()),
"invalid column comment");
}
if (schema.num_key_columns() <= 0) {
@@ -1711,8 +1727,9 @@ Status CatalogManager::CreateTable(const
CreateTableRequestPB* orig_req,
Schema client_schema;
RETURN_NOT_OK(SchemaFromPB(req.schema(), &client_schema));
- RETURN_NOT_OK(SetupError(ValidateClientSchema(normalized_table_name,
req.owner(), client_schema),
- resp, MasterErrorPB::INVALID_SCHEMA));
+ RETURN_NOT_OK(SetupError(
+ ValidateClientSchema(normalized_table_name, req.owner(), req.comment(),
client_schema),
+ resp, MasterErrorPB::INVALID_SCHEMA));
if (client_schema.has_column_ids()) {
return SetupError(Status::InvalidArgument("user requests should not have
Column IDs"),
resp, MasterErrorPB::INVALID_SCHEMA);
@@ -1935,7 +1952,7 @@ Status CatalogManager::CreateTable(const
CreateTableRequestPB* orig_req,
if (hms_catalog_ && is_user_table) {
CHECK(rpc);
Status s = hms_catalog_->CreateTable(
- table->id(), normalized_table_name, GetClusterId(), req.owner(),
schema);
+ table->id(), normalized_table_name, GetClusterId(), req.owner(),
schema, req.comment());
if (!s.ok()) {
s = s.CloneAndPrepend(Substitute(
"failed to create HMS catalog entry for table $0",
table->ToString()));
@@ -2070,6 +2087,9 @@ scoped_refptr<TableInfo> CatalogManager::CreateTableInfo(
if (req.has_owner()) {
metadata->set_owner(req.owner());
}
+ if (req.has_comment()) {
+ metadata->set_comment(req.comment());
+ }
// Set the table limit
if (FLAGS_enable_table_write_limit) {
if (FLAGS_table_disk_size_limit != TableInfo::TABLE_WRITE_DEFAULT_LIMIT) {
@@ -2763,7 +2783,8 @@ Status CatalogManager::AlterTableRpc(const
AlterTableRequestPB& req,
normalized_new_table_name,
GetClusterId(),
l.data().owner(),
- schema);
+ schema,
+ l.data().comment());
if (PREDICT_TRUE(s.ok())) {
LOG(INFO) << Substitute("renamed table $0 in HMS: new name $1",
table->ToString(), normalized_new_table_name);
@@ -2807,6 +2828,7 @@ Status CatalogManager::AlterTableHms(const string&
table_id,
const string& table_name,
const optional<string>& new_table_name,
const optional<string>& new_table_owner,
+ const optional<string>& new_table_comment,
int64_t notification_log_event_id) {
AlterTableRequestPB req;
AlterTableResponsePB resp;
@@ -2818,6 +2840,9 @@ Status CatalogManager::AlterTableHms(const string&
table_id,
if (new_table_owner) {
req.set_new_table_owner(new_table_owner.get());
}
+ if (new_table_comment) {
+ req.set_new_table_comment(new_table_comment.get());
+ }
// Use empty user to skip the authorization validation since the operation
// originates from catalog manager. Moreover, this avoids duplicate effort,
@@ -2961,9 +2986,9 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
DCHECK_EQ(new_schema.find_column_by_id(next_col_id),
static_cast<int>(Schema::kColumnNotFound));
- // Just validate the schema, not the name and owner (validated below).
+ // Just validate the schema, not the name, owner, or comment (validated
below).
RETURN_NOT_OK(SetupError(
- ValidateClientSchema(none, none, new_schema),
+ ValidateClientSchema(none, none, none, new_schema),
resp, MasterErrorPB::INVALID_SCHEMA));
// 4. Validate and try to acquire the new table name.
@@ -3025,7 +3050,15 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
l.mutable_data()->pb.set_owner(req.new_table_owner());
}
- // 6. Alter table partitioning.
+ // 6. Alter the table comment.
+ if (req.has_new_table_comment()) {
+ RETURN_NOT_OK(SetupError(
+ ValidateTableComment(req.new_table_comment()).CloneAndPrepend("invalid
table comment"),
+ resp, MasterErrorPB::INVALID_SCHEMA));
+ l.mutable_data()->pb.set_comment(req.new_table_comment());
+ }
+
+ // 7. Alter table partitioning.
vector<scoped_refptr<TabletInfo>> tablets_to_add;
vector<scoped_refptr<TabletInfo>> tablets_to_drop;
if (!alter_partitioning_steps.empty()) {
@@ -3039,7 +3072,7 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
resp, MasterErrorPB::UNKNOWN_ERROR));
}
- // 7. Alter table's extra configuration properties.
+ // 8. Alter table's extra configuration properties.
if (!req.new_extra_configs().empty()) {
TRACE("Apply alter extra-config");
Map<string, string> new_extra_configs;
@@ -3055,12 +3088,12 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
// Set to true if columns are altered, added or dropped.
bool has_schema_changes = !alter_schema_steps.empty();
- // Set to true if there are schema changes, the table is renamed, the owner
changed,
- // or extra configuration has changed.
+ // Set to true if there are schema changes, the table is renamed,
+ // or if any other table properties changed.
bool has_metadata_changes = has_schema_changes ||
req.has_new_table_name() || req.has_new_table_owner() ||
!req.new_extra_configs().empty() || req.has_disk_size_limit() ||
- req.has_row_count_limit();
+ req.has_row_count_limit() || req.has_new_table_comment();
// Set to true if there are partitioning changes.
bool has_partitioning_changes = !alter_partitioning_steps.empty();
// Set to true if metadata changes need to be applied to existing tablets.
@@ -3072,7 +3105,7 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
return Status::OK();
}
- // 8. Serialize the schema and increment the version number.
+ // 9. Serialize the schema and increment the version number.
if (has_metadata_changes_for_existing_tablets &&
!l.data().pb.has_fully_applied_schema()) {
l.mutable_data()->pb.mutable_fully_applied_schema()->CopyFrom(l.data().pb.schema());
}
@@ -3097,7 +3130,7 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
TabletMetadataGroupLock tablets_to_add_lock(LockMode::WRITE);
TabletMetadataGroupLock tablets_to_drop_lock(LockMode::RELEASED);
- // 9. Update sys-catalog with the new table schema and tablets to add/drop.
+ // 10. Update sys-catalog with the new table schema and tablets to add/drop.
TRACE("Updating metadata on disk");
{
SysCatalogTable::Actions actions;
@@ -3127,7 +3160,7 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
}
}
- // 10. Commit the in-memory state.
+ // 11. Commit the in-memory state.
TRACE("Committing alterations to in-memory state");
{
// Commit new tablet in-memory state. This doesn't require taking the
global
@@ -3183,18 +3216,19 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
// the tablet again.
tablets_to_drop_lock.Commit();
- // If there are schema changes or the owner changed, then update the entry
in the Hive Metastore.
- // This is done on a best-effort basis, since Kudu is the source of truth for
- // table schema information, and the table has already been altered in the
- // Kudu catalog via the successful sys-table write above.
- if (hms_catalog_ && (has_schema_changes || req.has_new_table_owner())) {
+ // If there are schema changes or the owner or comment changed, then update
the
+ // entry in the Hive Metastore. This is done on a best-effort basis, since
Kudu
+ // is the source of truth for table schema information, and the table has
already
+ // been altered in the Kudu catalog via the successful sys-table write above.
+ if (hms_catalog_ && (has_schema_changes ||
+ req.has_new_table_owner() || req.has_new_table_comment())) {
// Sanity check: if there are schema changes then this is necessarily not a
// table rename, since we split out the rename portion into its own
// 'transaction' which is serialized through the HMS.
DCHECK(!req.has_new_table_name());
auto s = hms_catalog_->AlterTable(
table->id(), normalized_table_name, normalized_table_name,
- GetClusterId(), l.mutable_data()->owner(), new_schema);
+ GetClusterId(), l.mutable_data()->owner(), new_schema,
l.mutable_data()->comment());
if (PREDICT_TRUE(s.ok())) {
LOG(INFO) << Substitute(
"altered HMS schema for table $0", table->ToString());
@@ -3218,7 +3252,7 @@ Status CatalogManager::AlterTable(const
AlterTableRequestPB& req,
SendDeleteTabletRequest(tablet, l, deletion_msg);
}
- // 10. Invalidate/purge corresponding entries in the table locations cache.
+ // 12. Invalidate/purge corresponding entries in the table locations cache.
if (table_locations_cache_ &&
(!tablets_to_add.empty() || !tablets_to_drop.empty())) {
table_locations_cache_->Remove(table->id());
@@ -3304,6 +3338,7 @@ Status CatalogManager::GetTableSchema(const
GetTableSchemaRequestPB* req,
resp->mutable_partition_schema()->CopyFrom(l.data().pb.partition_schema());
resp->set_table_name(l.data().pb.name());
resp->set_owner(l.data().pb.owner());
+ resp->set_comment(l.data().pb.comment());
RETURN_NOT_OK(ExtraConfigPBToPBMap(l.data().pb.extra_config(),
resp->mutable_extra_configs()));
diff --git a/src/kudu/master/catalog_manager.h
b/src/kudu/master/catalog_manager.h
index 565ee6c..0ffc79d 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -255,6 +255,11 @@ struct PersistentTableInfo {
return pb.owner();
}
+ // Return the table's comment.
+ const std::string& comment() const {
+ return pb.comment();
+ }
+
// Helper to set the state of the tablet with a custom message.
void set_state(SysTablesEntryPB::State state, const std::string& msg);
@@ -627,6 +632,7 @@ class CatalogManager : public
tserver::TabletReplicaLookupIf {
const std::string& table_name,
const boost::optional<std::string>& new_table_name,
const boost::optional<std::string>& new_table_owner,
+ const boost::optional<std::string>& new_table_comment,
int64_t notification_log_event_id) WARN_UNUSED_RESULT;
// Get the information about an in-progress alter operation. If 'user' is
diff --git a/src/kudu/master/hms_notification_log_listener.cc
b/src/kudu/master/hms_notification_log_listener.cc
index 82f906e..b586ad9 100644
--- a/src/kudu/master/hms_notification_log_listener.cc
+++ b/src/kudu/master/hms_notification_log_listener.cc
@@ -378,7 +378,16 @@ Status
HmsNotificationLogListenerTask::HandleAlterTableEvent(const hive::Notific
new_table_owner = after_table.owner;
}
- if (!new_table_name && !new_table_owner) {
+ optional<string> new_table_comment;
+ const string before_table_comment =
+ FindWithDefault(before_table.parameters,
hms::HmsClient::kTableCommentKey, "");
+ const string after_table_comment =
+ FindWithDefault(after_table.parameters,
hms::HmsClient::kTableCommentKey, "");
+ if (before_table_comment != after_table_comment) {
+ new_table_comment = after_table_comment;
+ }
+
+ if (!new_table_name && !new_table_owner && !new_table_comment) {
VLOG(2) << "Ignoring alter table event on table "
<< *table_id << " " << before_table_name;
return Status::OK();
@@ -388,6 +397,7 @@ Status
HmsNotificationLogListenerTask::HandleAlterTableEvent(const hive::Notific
before_table_name,
new_table_name,
new_table_owner,
+ new_table_comment,
event.eventId));
*durable_event_id = event.eventId;
return Status::OK();
diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc
index 402f688..b7076e3 100644
--- a/src/kudu/master/master-test.cc
+++ b/src/kudu/master/master-test.cc
@@ -43,6 +43,7 @@
#include "kudu/common/common.pb.h"
#include "kudu/common/partial_row.h"
#include "kudu/common/row_operations.h"
+#include "kudu/common/row_operations.pb.h"
#include "kudu/common/schema.h"
#include "kudu/common/types.h"
#include "kudu/common/wire_protocol.h"
@@ -125,6 +126,7 @@ DECLARE_bool(raft_prepare_replacement_before_eviction);
DECLARE_double(sys_catalog_fail_during_write);
DECLARE_int32(diagnostics_log_stack_traces_interval_ms);
DECLARE_int32(master_inject_latency_on_tablet_lookups_ms);
+DECLARE_int32(max_table_comment_length);
DECLARE_int32(rpc_service_queue_length);
DECLARE_int64(live_row_count_for_testing);
DECLARE_int64(on_disk_size_for_testing);
@@ -180,6 +182,7 @@ class MasterTest : public KuduTest {
const vector<KuduPartialRow>& split_rows,
const vector<pair<KuduPartialRow, KuduPartialRow>>&
bounds,
const optional<string>& owner,
+ const optional<string>& comment = boost::none,
const optional<TableTypePB>& table_type = boost::none,
const vector<vector<HashBucketSchema>>& range_hash_schema
= {});
@@ -547,7 +550,7 @@ Status MasterTest::CreateTable(const string& table_name,
RETURN_NOT_OK(split2.SetInt32("key", 20));
return CreateTable(
- table_name, schema, { split1, split2 }, {}, boost::none, table_type);
+ table_name, schema, { split1, split2 }, {}, boost::none, boost::none,
table_type);
}
Status MasterTest::CreateTable(const string& table_name,
@@ -555,6 +558,7 @@ Status MasterTest::CreateTable(const string& table_name,
const vector<KuduPartialRow>& split_rows,
const vector<pair<KuduPartialRow,
KuduPartialRow>>& bounds,
const optional<string>& owner,
+ const optional<string>& comment,
const optional<TableTypePB>& table_type,
const vector<vector<HashBucketSchema>>&
range_hash_schema) {
CreateTableRequestPB req;
@@ -590,6 +594,9 @@ Status MasterTest::CreateTable(const string& table_name,
if (owner) {
req.set_owner(*owner);
}
+ if (comment) {
+ req.set_comment(*comment);
+ }
if (!bounds.empty()) {
controller.RequireServerFeature(MasterFeatures::RANGE_PARTITION_BOUNDS);
}
@@ -838,7 +845,7 @@ TEST_F(MasterTest, TestCreateTableCheckRangeInvariants) {
ASSERT_OK(a_upper.SetInt32("key", 100));
vector<vector<HashBucketSchema>> range_hash_schema = {
vector<HashBucketSchema>() };
Status s = CreateTable(kTableName, kTableSchema, { split1 }, { { a_lower,
a_upper } },
- none, none, range_hash_schema);
+ none, none, none, range_hash_schema);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_STR_CONTAINS(s.ToString(),
"Both 'split_rows' and 'range_hash_schemas' cannot be "
@@ -857,7 +864,7 @@ TEST_F(MasterTest, TestCreateTableCheckRangeInvariants) {
vector<vector<HashBucketSchema>> range_hash_schema =
{std::move(hash_schemas_2),
std::move(hash_schemas_4)};
Status s = CreateTable(kTableName, kTableSchema, { }, { { a_lower, a_upper
} },
- none, none, range_hash_schema);
+ none, none, none, range_hash_schema);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_STR_CONTAINS(s.ToString(),
"The number of range bounds does not match the number
of per "
@@ -996,6 +1003,15 @@ TEST_F(MasterTest, TestCreateTableOwnerNameTooLong) {
ASSERT_STR_CONTAINS(s.ToString(), "invalid owner name");
}
+TEST_F(MasterTest, TestCreateTableCommentTooLong) {
+ constexpr const char* const kTableName = "testb";
+ const string kComment = string(FLAGS_max_table_comment_length + 1, 'x');
+ const Schema kTableSchema({ ColumnSchema("key", INT32), ColumnSchema("val",
INT32) }, 1);
+ Status s = CreateTable(kTableName, kTableSchema, {}, {}, none, kComment);
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "invalid table comment");
+}
+
// Regression test for KUDU-253/KUDU-592: crash if the schema passed to
CreateTable
// is invalid.
TEST_F(MasterTest, TestCreateTableInvalidSchema) {
diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto
index f1e8a48..92b1752 100644
--- a/src/kudu/master/master.proto
+++ b/src/kudu/master/master.proto
@@ -211,6 +211,9 @@ message SysTablesEntryPB {
optional int64 table_disk_size_limit = 15;
// Table row count limit.
optional int64 table_row_count_limit = 16;
+
+ // The comment on the table.
+ optional string comment = 17;
}
// The on-disk entry in the sys.catalog table ("metadata" column) to represent
@@ -533,6 +536,9 @@ message CreateTableRequestPB {
// The table type. If not set, it is assumed this table is a user-defined
// table, rather than a system table.
optional TableTypePB table_type = 11;
+
+ // The comment on the table.
+ optional string comment = 13;
}
message CreateTableResponsePB {
@@ -725,6 +731,8 @@ message AlterTableRequestPB {
optional int64 disk_size_limit = 8;
optional int64 row_count_limit = 9;
+
+ optional string new_table_comment = 10;
}
message AlterTableResponsePB {
@@ -791,6 +799,9 @@ message GetTableSchemaResponsePB {
// The user that owns the table.
optional string owner = 10;
+
+ // The comment on the table.
+ optional string comment = 11;
}
message ConnectToMasterRequestPB {
diff --git a/src/kudu/master/master_path_handlers.cc
b/src/kudu/master/master_path_handlers.cc
index 32bafdf..9012bec 100644
--- a/src/kudu/master/master_path_handlers.cc
+++ b/src/kudu/master/master_path_handlers.cc
@@ -261,6 +261,7 @@ void MasterPathHandlers::HandleCatalogManager(const
Webserver::WebRequest& req,
table_json["name"] = EscapeForHtmlToString(l.data().name());
table_json["id"] = EscapeForHtmlToString(table->id());
table_json["owner"] = EscapeForHtmlToString(l.data().owner());
+ table_json["comment"] = EscapeForHtmlToString(l.data().comment());
table_json["state"] = state;
table_json["message"] = EscapeForHtmlToString(l.data().pb.state_msg());
table_json["tablet_count"] =
HumanReadableInt::ToString(table->num_tablets());
@@ -357,6 +358,7 @@ void MasterPathHandlers::HandleTablePage(const
Webserver::WebRequest& req,
TableMetadataLock l(table.get(), LockMode::READ);
(*output)["name"] = l.data().name();
(*output)["owner"] = l.data().owner();
+ (*output)["comment"] = l.data().comment();
// Not all Kudu tablenames are also valid Impala identifiers. We need to
// replace such names with a placeholder when they are used as Impala
diff --git a/src/kudu/tools/kudu-admin-test.cc
b/src/kudu/tools/kudu-admin-test.cc
index 208c64b..daafb99 100644
--- a/src/kudu/tools/kudu-admin-test.cc
+++ b/src/kudu/tools/kudu-admin-test.cc
@@ -1756,7 +1756,8 @@ TEST_F(AdminCliTest, TestDescribeTable) {
"\n"
")\n"
"OWNER alice\n"
- "REPLICAS 1");
+ "REPLICAS 1\n"
+ "COMMENT ");
// Test a table with all types in its schema, multiple hash partitioning
// levels, multiple range partitions, and non-covered ranges.
@@ -1821,6 +1822,7 @@ TEST_F(AdminCliTest, TestDescribeTable) {
.add_range_partition(lower_bound1.release(),
upper_bound1.release())
.num_replicas(FLAGS_num_replicas)
.set_owner("alice")
+ .set_comment("table comment")
.Create());
}
@@ -1862,7 +1864,8 @@ TEST_F(AdminCliTest, TestDescribeTable) {
" PARTITION 2 <= VALUES < 3\n"
")\n"
"OWNER alice\n"
- "REPLICAS 1");
+ "REPLICAS 1\n"
+ "COMMENT table comment");
// Test the describe output with `-show_attributes=true`.
stdout.clear();
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index c475511..e5057c3 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -4094,6 +4094,7 @@ Status AlterHmsWithReplacedParam(HmsClient* hms_client,
TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
string kUsername = "alice";
string kOtherUsername = "bob";
+ string kOtherComment = "a table comment";
ExternalMiniClusterOptions opts;
opts.hms_mode = HmsMode::DISABLE_HIVE_METASTORE;
opts.enable_kerberos = EnableKerberos();
@@ -4136,7 +4137,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.control", &control));
ASSERT_OK(hms_catalog.CreateTable(
control->id(), control->name(), kudu_client->cluster_id(), kUsername,
- KuduSchema::ToSchema(control->schema())));
+ KuduSchema::ToSchema(control->schema()), control->comment()));
// Control case: the check tool should not flag this external synchronized
table.
shared_ptr<KuduTable> control_external;
@@ -4144,7 +4145,8 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.control_external",
&control_external));
ASSERT_OK(hms_catalog.CreateTable(
control_external->id(), control_external->name(),
kudu_client->cluster_id(),
- kUsername, KuduSchema::ToSchema(control_external->schema()),
HmsClient::kExternalTable));
+ kUsername, KuduSchema::ToSchema(control_external->schema()),
control_external->comment(),
+ HmsClient::kExternalTable));
hive::Table hms_control_external;
ASSERT_OK(hms_client.GetTable("default", "control_external",
&hms_control_external));
hms_control_external.parameters[HmsClient::kKuduTableIdKey] =
control_external->id();
@@ -4159,7 +4161,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.UPPERCASE", &test_uppercase));
ASSERT_OK(hms_catalog.CreateTable(
test_uppercase->id(), test_uppercase->name(),
kudu_client->cluster_id(),
- kUsername, KuduSchema::ToSchema(test_uppercase->schema())));
+ kUsername, KuduSchema::ToSchema(test_uppercase->schema()),
test_uppercase->comment()));
// Test case: inconsistent schema.
shared_ptr<KuduTable> inconsistent_schema;
@@ -4167,7 +4169,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.inconsistent_schema",
&inconsistent_schema));
ASSERT_OK(hms_catalog.CreateTable(
inconsistent_schema->id(), inconsistent_schema->name(),
kudu_client->cluster_id(),
- kUsername, SchemaBuilder().Build()));
+ kUsername, SchemaBuilder().Build(), inconsistent_schema->comment()));
// Test case: inconsistent name.
shared_ptr<KuduTable> inconsistent_name;
@@ -4175,7 +4177,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.inconsistent_name",
&inconsistent_name));
ASSERT_OK(hms_catalog.CreateTable(
inconsistent_name->id(), "default.inconsistent_name_hms",
kudu_client->cluster_id(),
- kUsername, KuduSchema::ToSchema(inconsistent_name->schema())));
+ kUsername, KuduSchema::ToSchema(inconsistent_name->schema()),
inconsistent_name->comment()));
// Test case: inconsistent cluster id.
shared_ptr<KuduTable> inconsistent_cluster;
@@ -4183,7 +4185,8 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.inconsistent_cluster",
&inconsistent_cluster));
ASSERT_OK(hms_catalog.CreateTable(
inconsistent_cluster->id(), "default.inconsistent_cluster",
"inconsistent_cluster",
- kUsername, KuduSchema::ToSchema(inconsistent_cluster->schema())));
+ kUsername, KuduSchema::ToSchema(inconsistent_cluster->schema()),
+ inconsistent_cluster->comment()));
// Test case: inconsistent master addresses.
shared_ptr<KuduTable> inconsistent_master_addrs;
@@ -4196,7 +4199,8 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(invalid_hms_catalog.CreateTable(
inconsistent_master_addrs->id(), inconsistent_master_addrs->name(),
kudu_client->cluster_id(), kUsername,
- KuduSchema::ToSchema(inconsistent_master_addrs->schema())));
+ KuduSchema::ToSchema(inconsistent_master_addrs->schema()),
+ inconsistent_master_addrs->comment()));
// Test case: bad table id.
shared_ptr<KuduTable> bad_id;
@@ -4204,18 +4208,18 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.bad_id", &bad_id));
ASSERT_OK(hms_catalog.CreateTable(
"not_a_table_id", "default.bad_id", kudu_client->cluster_id(),
- kUsername, KuduSchema::ToSchema(bad_id->schema())));
+ kUsername, KuduSchema::ToSchema(bad_id->schema()), bad_id->comment()));
// Test case: orphan table in the HMS.
ASSERT_OK(hms_catalog.CreateTable(
"orphan-hms-table-id", "default.orphan_hms_table",
"orphan-hms-cluster-id",
- kUsername, SchemaBuilder().Build()));
+ kUsername, SchemaBuilder().Build(), ""));
// Test case: orphan table in the HMS with different, but functionally
// the same master addresses.
ASSERT_OK(hms_catalog.CreateTable(
"orphan-hms-masters-id", "default.orphan_hms_table_masters",
"orphan-hms-cluster-id",
- kUsername, SchemaBuilder().Build()));
+ kUsername, SchemaBuilder().Build(), ""));
// Reverse the address order and duplicate the addresses.
vector<string> modified_addrs;
for (const auto& hp : cluster_->master_rpc_addrs()) {
@@ -4231,7 +4235,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(hms_catalog.CreateTable(
"orphan-hms-table-id-external", "default.orphan_hms_table_external",
"orphan-hms-cluster-id-external", kUsername,
- SchemaBuilder().Build(), HmsClient::kExternalTable));
+ SchemaBuilder().Build(), "", HmsClient::kExternalTable));
hive::Table hms_orphan_external;
ASSERT_OK(hms_client.GetTable("default", "orphan_hms_table_external",
&hms_orphan_external));
hms_orphan_external.parameters[HmsClient::kExternalPurgeKey] = "true";
@@ -4299,7 +4303,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.no_owner_in_hms",
&no_owner_in_hms));
ASSERT_OK(hms_catalog.CreateTable(
no_owner_in_hms->id(), no_owner_in_hms->name(),
kudu_client->cluster_id(),
- boost::none, KuduSchema::ToSchema(no_owner_in_hms->schema())));
+ boost::none, KuduSchema::ToSchema(no_owner_in_hms->schema()),
no_owner_in_hms->comment()));
// Test case: no owner in Kudu
shared_ptr<KuduTable> no_owner_in_kudu;
@@ -4307,7 +4311,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.no_owner_in_kudu",
&no_owner_in_kudu));
ASSERT_OK(hms_catalog.CreateTable(
no_owner_in_kudu->id(), no_owner_in_kudu->name(),
kudu_client->cluster_id(),
- kUsername, KuduSchema::ToSchema(no_owner_in_kudu->schema())));
+ kUsername, KuduSchema::ToSchema(no_owner_in_kudu->schema()),
no_owner_in_kudu->comment()));
// Test case: different owner in Kudu and HMS
shared_ptr<KuduTable> different_owner;
@@ -4315,7 +4319,17 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
ASSERT_OK(kudu_client->OpenTable("default.different_owner",
&different_owner));
ASSERT_OK(hms_catalog.CreateTable(
different_owner->id(), different_owner->name(),
kudu_client->cluster_id(),
- kOtherUsername, KuduSchema::ToSchema(different_owner->schema())));
+ kOtherUsername, KuduSchema::ToSchema(different_owner->schema()),
+ different_owner->comment()));
+
+ // Test case: different comment in Kudu and HMS
+ shared_ptr<KuduTable> different_comment;
+ ASSERT_OK(CreateKuduTable(kudu_client, "default.different_comment",
kUsername));
+ ASSERT_OK(kudu_client->OpenTable("default.different_comment",
&different_comment));
+ ASSERT_OK(hms_catalog.CreateTable(
+ different_comment->id(), different_comment->name(),
kudu_client->cluster_id(),
+ different_comment->owner(),
KuduSchema::ToSchema(different_comment->schema()),
+ kOtherComment));
unordered_set<string> consistent_tables = {
"default.control",
@@ -4329,6 +4343,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
"default.inconsistent_cluster",
"default.inconsistent_master_addrs",
"default.bad_id",
+ "default.different_comment",
"default.different_owner",
"default.no_owner_in_hms",
"default.no_owner_in_kudu",
@@ -4432,6 +4447,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
"default.inconsistent_cluster",
"default.inconsistent_master_addrs",
"default.bad_id",
+ "default.different_comment",
"default.different_owner",
"default.no_owner_in_hms",
"default.no_owner_in_kudu",
@@ -4468,6 +4484,7 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndAutomaticFixHmsMetadata) {
"default.bad_id",
"default.control",
"default.control_external",
+ "default.different_comment",
"default.different_owner",
"default.inconsistent_cluster",
"default.inconsistent_master_addrs",
@@ -4536,11 +4553,11 @@ TEST_P(ToolTestKerberosParameterized,
TestCheckAndManualFixHmsMetadata) {
ASSERT_OK(hms_catalog.CreateTable(
duplicate_hms_tables->id(), "default.duplicate_hms_tables",
kudu_client->cluster_id(), kUsername,
- KuduSchema::ToSchema(duplicate_hms_tables->schema())));
+ KuduSchema::ToSchema(duplicate_hms_tables->schema()),
duplicate_hms_tables->comment()));
ASSERT_OK(hms_catalog.CreateTable(
duplicate_hms_tables->id(), "default.duplicate_hms_tables_2",
kudu_client->cluster_id(), kUsername,
- KuduSchema::ToSchema(duplicate_hms_tables->schema())));
+ KuduSchema::ToSchema(duplicate_hms_tables->schema()),
duplicate_hms_tables->comment()));
// Test case: Kudu tables Hive-incompatible names.
ASSERT_OK(CreateKuduTable(kudu_client, "default.hive-incompatible-name"));
@@ -4821,11 +4838,11 @@ TEST_F(ToolTest, TestHmsList) {
ASSERT_OK(hms_catalog.CreateTable(
"1", "default.table1", "cluster-id", kUsername,
KuduSchema::ToSchema(simple_table->schema()),
- hms::HmsClient::kManagedTable));
+ "comment 1", hms::HmsClient::kManagedTable));
ASSERT_OK(hms_catalog.CreateTable(
"2", "default.table2", "cluster-id", boost::none,
KuduSchema::ToSchema(simple_table->schema()),
- hms::HmsClient::kExternalTable));
+ "", hms::HmsClient::kExternalTable));
// Test the output when HMS integration is disabled.
string err;
@@ -4850,10 +4867,11 @@ TEST_F(ToolTest, TestHmsList) {
"default | table2 | EXTERNAL_TABLE | default.table2");
// Run the list tool filtering columns and using a different format.
- RunActionStdoutString(Substitute("hms list --columns
table,owner,kudu.table_id, --format=csv $0",
- master_addr), &out);
- ASSERT_STR_CONTAINS(out, "table1,alice,1");
- ASSERT_STR_CONTAINS(out, "table2,,");
+ RunActionStdoutString(
+ Substitute("hms list --columns table,owner,comment,kudu.table_id,
--format=csv $0",
+ master_addr), &out);
+ ASSERT_STR_CONTAINS(out, "table1,alice,comment 1,1");
+ ASSERT_STR_CONTAINS(out, "table2,,,");
}
// This test is parameterized on the serialization mode and Kerberos.
diff --git a/src/kudu/tools/tool_action_hms.cc
b/src/kudu/tools/tool_action_hms.cc
index 1286f4f..b554967 100644
--- a/src/kudu/tools/tool_action_hms.cc
+++ b/src/kudu/tools/tool_action_hms.cc
@@ -116,6 +116,16 @@ Status ChangeOwnerInKuduCatalog(KuduClient* kudu_client,
->Alter();
}
+// Only alter the table in Kudu but not in the Hive Metastore.
+Status ChangeTableCommentInKuduCatalog(KuduClient* kudu_client,
+ const string& name,
+ const string& comment) {
+ unique_ptr<KuduTableAlterer> alterer(kudu_client->NewTableAlterer(name));
+ return alterer->SetComment(comment)
+ ->modify_external_catalogs(false)
+ ->Alter();
+}
+
Status Init(const RunnerContext& context,
shared_ptr<KuduClient>* kudu_client,
unique_ptr<HmsCatalog>* hms_catalog,
@@ -189,8 +199,9 @@ bool IsSynced(const set<string>& master_addresses,
return false;
}
Status s = HmsCatalog::PopulateTable(kudu_table.id(), kudu_table.name(),
kudu_table.owner(),
- schema,
kudu_table.client()->cluster_id(),
- *hms_masters_field,
hms_table.tableType, &hms_table_copy);
+ schema, kudu_table.comment(),
+ kudu_table.client()->cluster_id(),
*hms_masters_field,
+ hms_table.tableType, &hms_table_copy);
return s.ok() && hms_table_copy == hms_table;
}
@@ -287,6 +298,10 @@ Status PrintHMSTables(vector<hive::Table> tables, ostream&
out) {
for (auto& hms_table : tables) {
values.emplace_back(hms_table.owner);
}
+ } else if (iequals(column.ToString(), "comment")) {
+ for (auto& hms_table : tables) {
+ values.emplace_back(hms_table.parameters[HmsClient::kTableCommentKey]);
+ }
} else if (iequals(column.ToString(), HmsClient::kKuduTableNameKey)) {
for (auto& hms_table : tables) {
values.emplace_back(hms_table.parameters[HmsClient::kKuduTableNameKey]);
@@ -665,7 +680,7 @@ Status FixHmsMetadata(const RunnerContext& context) {
LOG(INFO) << "[dryrun] Creating HMS table for Kudu table " <<
TableIdent(*kudu_table);
} else {
Status s = hms_catalog->CreateTable(table_id, table_name, cluster_id,
- kudu_table->owner(), schema);
+ kudu_table->owner(), schema, kudu_table->comment());
if (s.IsAlreadyPresent()) {
LOG(ERROR) << "Failed to create HMS table for Kudu table "
<< TableIdent(*kudu_table)
@@ -720,7 +735,8 @@ Status FixHmsMetadata(const RunnerContext& context) {
} else {
RETURN_NOT_OK_PREPEND(hms_catalog->UpgradeLegacyImpalaTable(
kudu_table.id(), kudu_table.client()->cluster_id(),
hms_table.dbName,
- hms_table.tableName,
KuduSchema::ToSchema(kudu_table.schema())),
+ hms_table.tableName,
KuduSchema::ToSchema(kudu_table.schema()),
+ kudu_table.comment()),
Substitute("failed to upgrade legacy Impala HMS metadata for table
$0",
hms_table_name));
}
@@ -761,6 +777,7 @@ Status FixHmsMetadata(const RunnerContext& context) {
const hive::Table& hms_table = table_pair.second;
string hms_table_name = Substitute("$0.$1", hms_table.dbName,
hms_table.tableName);
string owner = kudu_table.owner();
+ string comment = kudu_table.comment();
if (hms_table_name != kudu_table.name()) {
// Update the Kudu table name to match the HMS table name.
@@ -799,6 +816,24 @@ Status FixHmsMetadata(const RunnerContext& context) {
}
}
+ // If the HMS table has a table comment and Kudu does not, update the
Kudu table comment
+ // to match the HMS table comment. Otherwise the metadata step below
will ensure the Kudu
+ // comment is updated in the HMS.
+ const string hms_table_comment =
+ FindWithDefault(hms_table.parameters,
hms::HmsClient::kTableCommentKey, "");
+ if (hms_table_comment != comment && comment.empty()) {
+ if (FLAGS_dryrun) {
+ LOG(INFO) << "[dryrun] Changing table comment for " <<
TableIdent(kudu_table)
+ << " to " << hms_table_comment << " in Kudu catalog.";
+ } else {
+
RETURN_NOT_OK_PREPEND(ChangeTableCommentInKuduCatalog(kudu_client.get(),
+ kudu_table.name(), hms_table_comment),
+ Substitute("failed to change table comment of $0 to $1 in Kudu
catalog",
+ TableIdent(kudu_table), hms_table_comment));
+ comment = hms_table_comment;
+ }
+ }
+
// Update the HMS table metadata to match Kudu.
if (FLAGS_dryrun) {
LOG(INFO) << "[dryrun] Refreshing HMS table metadata for Kudu table "
@@ -809,7 +844,7 @@ Status FixHmsMetadata(const RunnerContext& context) {
// Disable table ID checking to support fixing tables with
unsynchronized IDs.
hms_catalog->AlterTable(kudu_table.id(), hms_table_name,
hms_table_name,
kudu_table.client()->cluster_id(), owner,
schema,
- /* check_id */ false),
+ comment, /* check_id */ false),
Substitute("failed to refresh HMS table metadata for Kudu table
$0",
TableIdent(kudu_table)));
}
@@ -938,7 +973,7 @@ unique_ptr<Mode> BuildHmsMode() {
HmsClient::kKuduTableNameKey),
Substitute("Comma-separated list of HMS entry
fields to "
"include in output.\nPossible values:
database, "
- "table, type, owner, $0, $1, $2, $3,
$4",
+ "table, type, owner, comment, $0, $1,
$2, $3, $4",
HmsClient::kKuduTableNameKey,
HmsClient::kKuduTableIdKey,
HmsClient::kKuduClusterIdKey,
diff --git a/src/kudu/tools/tool_action_table.cc
b/src/kudu/tools/tool_action_table.cc
index 333dcd6..656a342 100644
--- a/src/kudu/tools/tool_action_table.cc
+++ b/src/kudu/tools/tool_action_table.cc
@@ -236,6 +236,9 @@ Status DescribeTable(const RunnerContext& context) {
// Finally, the replication factor.
cout << "REPLICAS " << table->num_replicas() << endl;
+ // The comment.
+ cout << "COMMENT " << table->comment() << endl;
+
return Status::OK();
}
diff --git a/www/table.mustache b/www/table.mustache
index 4b7aee4..87cfa19 100644
--- a/www/table.mustache
+++ b/www/table.mustache
@@ -30,6 +30,7 @@ under the License.
<table class="table table-striped">
<tbody>
<tr><td>Owner:</td><td>{{owner}}</td></tr>
+ <tr><td>Comment:</td><td>{{comment}}</td></tr>
<tr><td>Version:</td><td>{{version}}</td></tr>
<tr><td>State:</td><td>{{state}}
{{#state_msg}}({{.}}){{/state_msg}}</td></tr>
<tr><td>Column Count:</td><td>{{column_count}}</td></tr>
