Repository: kudu Updated Branches: refs/heads/master 4eefc8d4e -> cee17c03b
KUDU-2191: downcase/normalize table names during DDL This is a followup to 7b048b8dbe which changed the catalog manager to be case preserving, but insensitive on lookup when the HMS integration is enabled. It turns out this was only possible because the HMS failed to downcase/normalize table names in notification log events[1]. This is an oversight, and probably could be considered a bug, and HMS developers have suggested that Kudu should not rely on it. After a lot of consideration I haven't been able to come up with a way to keep the case preserving semantics without changes to the HMS APIs, so instead this commit throws in the towel and adopts HMS-style case normalization during CREATE TABLE and ALTER TABLE RENAME operations. Existing tables with uppercase table names will not be altered automatically (this is consistent with the current handling of non-ascii chars in table names), so the upgrade CLI tool will be extended in a follow up commit to handle this. [1] In particular, renaming a table is problematic if the notification log listener events doesn't preserve case due to how the catalog manager / notification log listener handles table renames. Change-Id: Ie32a209d9d85851562691ddbc30f7dd02886bad7 Reviewed-on: http://gerrit.cloudera.org:8080/10903 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo <a...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/cee17c03 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/cee17c03 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/cee17c03 Branch: refs/heads/master Commit: cee17c03bc30037bf2a7d97c6bb3aa39cec34a4c Parents: 4eefc8d Author: Dan Burkert <danburk...@apache.org> Authored: Tue Jul 10 11:07:03 2018 -0700 Committer: Dan Burkert <danburk...@apache.org> Committed: Wed Jul 11 23:01:51 2018 +0000 ---------------------------------------------------------------------- src/kudu/integration-tests/master_hms-itest.cc | 19 ++--- src/kudu/master/catalog_manager.cc | 91 +++++++++------------ 2 files changed, 46 insertions(+), 64 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/cee17c03/src/kudu/integration-tests/master_hms-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/master_hms-itest.cc b/src/kudu/integration-tests/master_hms-itest.cc index 4b639f5..2fd415c 100644 --- a/src/kudu/integration-tests/master_hms-itest.cc +++ b/src/kudu/integration-tests/master_hms-itest.cc @@ -499,23 +499,16 @@ TEST_F(MasterHmsTest, TestUppercaseIdentifiers) { ASSERT_EQ(name, table->name()); } - // Listing tables shows the preserved case. + // Listing tables shows the normalized case. vector<string> tables; ASSERT_OK(client_->ListTables(&tables)); - ASSERT_EQ(tables, vector<string>({ "default.MyTable" })); + ASSERT_EQ(tables, vector<string>({ "default.mytable" })); // Rename the table to the same normalized name, but with a different case. unique_ptr<KuduTableAlterer> table_alterer; table_alterer.reset(client_->NewTableAlterer("default.mytable")); - ASSERT_OK(table_alterer->RenameTo("DEFAULT.MYTABLE")->Alter()); - NO_FATALS(CheckTable("default", "MyTable")); - NO_FATALS(CheckTable("default", "mytable")); - NO_FATALS(CheckTable("default", "MYTABLE")); - - // The master should retain the new case. - tables.clear(); - ASSERT_OK(client_->ListTables(&tables)); - ASSERT_EQ(tables, vector<string>({ "DEFAULT.MYTABLE" })); + Status s = table_alterer->RenameTo("DEFAULT.MYTABLE")->Alter(); + ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString(); // Rename the table to something different. table_alterer.reset(client_->NewTableAlterer("DEFAULT.MYTABLE")); @@ -530,10 +523,10 @@ TEST_F(MasterHmsTest, TestUppercaseIdentifiers) { NO_FATALS(CheckTable("default", "AbC")); }); - // Listing tables shows the preserved case. + // Listing tables shows the normalized case. tables.clear(); ASSERT_OK(client_->ListTables(&tables)); - ASSERT_EQ(tables, vector<string>({ "default.AbC" })); + ASSERT_EQ(tables, vector<string>({ "default.abc" })); // Drop the table. ASSERT_OK(client_->DeleteTable("DEFAULT.abc")); http://git-wip-us.apache.org/repos/asf/kudu/blob/cee17c03/src/kudu/master/catalog_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 8f58924..b10a4cc 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -1363,10 +1363,9 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, // a. Validate the user request. Schema client_schema; RETURN_NOT_OK(SchemaFromPB(req.schema(), &client_schema)); - const string& table_name = req.name(); - string normalized_table_name = NormalizeTableName(table_name); + string normalized_table_name = NormalizeTableName(req.name()); - RETURN_NOT_OK(SetupError(ValidateClientSchema(table_name, client_schema), + RETURN_NOT_OK(SetupError(ValidateClientSchema(normalized_table_name, client_schema), resp, MasterErrorPB::INVALID_SCHEMA)); if (client_schema.has_column_ids()) { return SetupError(Status::InvalidArgument("user requests should not have Column IDs"), @@ -1493,7 +1492,7 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, "tablet replica of the newly created table $0 in case of a replica " "failure: $1 tablet servers are needed, $2 are alive. " "Consider bringing up additional tablet server(s)$3", - table_name, num_ts_needed_for_rereplication, num_live_tservers, + normalized_table_name, num_ts_needed_for_rereplication, num_live_tservers, is_off_by_one ? " or running both the masters and all tablet servers" " with --raft_prepare_replacement_before_eviction=false" " flag (not recommended)." : "."); @@ -1509,7 +1508,7 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, table = FindPtrOrNull(normalized_table_names_map_, normalized_table_name); if (table != nullptr) { return SetupError(Status::AlreadyPresent(Substitute( - "table $0 already exists with id $1", table_name, table->id())), + "table $0 already exists with id $1", normalized_table_name, table->id())), resp, MasterErrorPB::TABLE_ALREADY_PRESENT); } @@ -1521,7 +1520,7 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, // in the case of an error. Instead, we force the client to retry at a // later time. return SetupError(Status::ServiceUnavailable(Substitute( - "new table name $0 is already reserved", table_name)), + "new table name $0 is already reserved", normalized_table_name)), resp, MasterErrorPB::TABLE_ALREADY_PRESENT); } } @@ -1564,10 +1563,10 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, // It is critical that this step happen before writing the table to the sys catalog, // since this step validates that the table name is available in the HMS catalog. if (hms_catalog_) { - Status s = hms_catalog_->CreateTable(table->id(), req.name(), schema); + Status s = hms_catalog_->CreateTable(table->id(), normalized_table_name, schema); if (!s.ok()) { s = s.CloneAndPrepend(Substitute("an error occurred while creating table $0 in the HMS", - req.name())); + normalized_table_name)); LOG(WARNING) << s.ToString(); return SetupError(std::move(s), resp, MasterErrorPB::HIVE_METASTORE_ERROR); } @@ -1577,7 +1576,7 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, // TODO(dan): figure out how to test this. if (hms_catalog_) { TRACE("Rolling back HMS table creation"); - WARN_NOT_OK(hms_catalog_->DropTable(table->id(), req.name()), + WARN_NOT_OK(hms_catalog_->DropTable(table->id(), normalized_table_name), "an error occurred while attempting to delete orphaned HMS table entry"); } }); @@ -1661,7 +1660,7 @@ scoped_refptr<TableInfo> CatalogManager::CreateTableInfo(const CreateTableReques table->mutable_metadata()->StartMutation(); SysTablesEntryPB *metadata = &table->mutable_metadata()->mutable_dirty()->pb; metadata->set_state(SysTablesEntryPB::PREPARING); - metadata->set_name(req.name()); + metadata->set_name(NormalizeTableName(req.name())); metadata->set_version(0); metadata->set_next_column_id(ColumnId(schema.max_col_id() + 1)); metadata->set_num_replicas(req.num_replicas()); @@ -2163,15 +2162,16 @@ Status CatalogManager::AlterTableRpc(const AlterTableRequestPB& req, TableMetadataLock l; RETURN_NOT_OK(FindAndLockTable(req, resp, LockMode::READ, &table, &l)); RETURN_NOT_OK(CheckIfTableDeletedOrNotRunning(&l, resp)); + string normalized_new_table_name = NormalizeTableName(req.new_table_name()); // The HMS allows renaming a table to the same name (ALTER TABLE t RENAME TO t), // however Kudu does not, so we must enforce this constraint ourselves before // altering the table in the HMS. The comparison is on the non-normalized // table names, since we want to allow changing the case of a table name. - if (l.data().name() == req.new_table_name()) { + if (l.data().name() == normalized_new_table_name) { return SetupError( Status::AlreadyPresent(Substitute("table $0 already exists with id $1", - req.new_table_name(), table->id())), + normalized_new_table_name, table->id())), resp, MasterErrorPB::TABLE_ALREADY_PRESENT); } @@ -2180,7 +2180,7 @@ Status CatalogManager::AlterTableRpc(const AlterTableRequestPB& req, // Rename the table in the HMS. RETURN_NOT_OK(SetupError(hms_catalog_->AlterTable( - table->id(), l.data().name(), req.new_table_name(), + table->id(), l.data().name(), normalized_new_table_name, schema), resp, MasterErrorPB::HIVE_METASTORE_ERROR)); @@ -2262,7 +2262,6 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB& req, } string table_name = l.data().name(); - string normalized_table_name = NormalizeTableName(table_name); *resp->mutable_table_id() = table->id(); // 3. Calculate and validate new schema for the on-disk state, not persisted yet. @@ -2287,55 +2286,45 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB& req, resp, MasterErrorPB::INVALID_SCHEMA)); // 4. Validate and try to acquire the new table name. - bool do_cleanup = false; string normalized_new_table_name = NormalizeTableName(req.new_table_name()); if (req.has_new_table_name()) { RETURN_NOT_OK(SetupError( - ValidateIdentifier(req.new_table_name()).CloneAndPrepend("invalid table name"), + ValidateIdentifier(normalized_new_table_name).CloneAndPrepend("invalid table name"), resp, MasterErrorPB::INVALID_SCHEMA)); // Validate the new table name. - // - // Special case: the new table name and existing table names are different, - // but map to the same normalized name. In this case we don't need to - // reserve the new table name, since it's already reserved by way of - // existing in the by-name map. - if (!(table_name != req.new_table_name() && - normalized_table_name == normalized_new_table_name)) { - std::lock_guard<LockType> catalog_lock(lock_); - TRACE("Acquired catalog manager lock"); - - // Verify that the table does not exist. - scoped_refptr<TableInfo> other_table = FindPtrOrNull(normalized_table_names_map_, - normalized_new_table_name); - - if (other_table != nullptr) { - return SetupError( - Status::AlreadyPresent(Substitute("table $0 already exists with id $1", - req.new_table_name(), table->id())), - resp, MasterErrorPB::TABLE_ALREADY_PRESENT); - } + std::lock_guard<LockType> catalog_lock(lock_); + TRACE("Acquired catalog manager lock"); - // Reserve the new table name if possible. - if (!InsertIfNotPresent(&reserved_normalized_table_names_, normalized_new_table_name)) { - // ServiceUnavailable will cause the client to retry the create table - // request. We don't want to outright fail the request with - // 'AlreadyPresent', because a table name reservation can be rolled back - // in the case of an error. Instead, we force the client to retry at a - // later time. - return SetupError(Status::ServiceUnavailable(Substitute( - "table name $0 is already reserved", req.new_table_name())), - resp, MasterErrorPB::TABLE_ALREADY_PRESENT); - } - do_cleanup = true; + // Verify that the table does not exist. + scoped_refptr<TableInfo> other_table = FindPtrOrNull(normalized_table_names_map_, + normalized_new_table_name); + + if (other_table != nullptr) { + return SetupError( + Status::AlreadyPresent(Substitute("table $0 already exists with id $1", + normalized_new_table_name, table->id())), + resp, MasterErrorPB::TABLE_ALREADY_PRESENT); + } + + // Reserve the new table name if possible. + if (!InsertIfNotPresent(&reserved_normalized_table_names_, normalized_new_table_name)) { + // ServiceUnavailable will cause the client to retry the create table + // request. We don't want to outright fail the request with + // 'AlreadyPresent', because a table name reservation can be rolled back + // in the case of an error. Instead, we force the client to retry at a + // later time. + return SetupError(Status::ServiceUnavailable(Substitute( + "table name $0 is already reserved", normalized_new_table_name)), + resp, MasterErrorPB::TABLE_ALREADY_PRESENT); } - l.mutable_data()->pb.set_name(req.new_table_name()); + l.mutable_data()->pb.set_name(normalized_new_table_name); } // Ensure that we drop our reservation upon return. auto cleanup = MakeScopedCleanup([&] () { - if (do_cleanup) { + if (req.has_new_table_name()) { std::lock_guard<LockType> l(lock_); CHECK_EQ(1, reserved_normalized_table_names_.erase(normalized_new_table_name)); } @@ -2434,7 +2423,7 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB& req, // and tablets indices. std::lock_guard<LockType> lock(lock_); if (req.has_new_table_name()) { - if (normalized_table_names_map_.erase(normalized_table_name) != 1) { + if (normalized_table_names_map_.erase(table_name) != 1) { LOG(FATAL) << "Could not remove table " << table->ToString() << " from map in response to AlterTable request: " << SecureShortDebugString(req);