kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aw...@apache.org
Subject [kudu] branch master updated: hms: Adjust external table handling
Date Wed, 22 May 2019 22:47:46 GMT
This is an automated email from the ASF dual-hosted git repository.

awong 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 6a8d1d2  hms: Adjust external table handling
6a8d1d2 is described below

commit 6a8d1d29e5c395a61d84733a39793e28950a3693
Author: Grant Henke <granthenke@apache.org>
AuthorDate: Wed May 15 17:19:01 2019 -0500

    hms: Adjust external table handling
    
    This patch changes the handling of external HMS tables.
    Currently we synchronize external HMS entries and
    create all synchronized HMS entries as external.
    
    With this patch, we only synchronize managed HMS
    entries and create all entries as managed by default.
    This allows external tables to work the same regardless
    of HMS synchronization being enabled.
    
    This patch touches many locations in the code because
    the use and validation of the new behavior is tightly
    coupled.
    
    The changes include:
    - Prevent altering table type via the KuduMetastorePlugin
    - Ignore non-managed Kudu tables in the
      KuduMetastorePlugin
    - Ignore non-managed Kudu tables in the
      hms_notification_log_listener
    - Use managed tables by default
    - Always use the new storage format for new entries
    - Add the table_name property to the new storage format
    - Adjust upgrade and downgrade logic for external tables
    - Adjust HmsCatalog::PopulateTable logic for external
      tables
    - Adjust the tooling logic to correctly handle external
      tables.
    
    Note: The updates to tooling logic and testing was
    minimal and will be improved in follow on patches.
    
    Change-Id: I6b7495486fa86710afb670a8e12f72684ba7af8f
    Reviewed-on: http://gerrit.cloudera.org:8080/13347
    Reviewed-by: Andrew Wong <awong@cloudera.com>
    Tested-by: Andrew Wong <awong@cloudera.com>
    Reviewed-by: Mike Percy <mpercy@apache.org>
    Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
---
 .../kudu/hive/metastore/KuduMetastorePlugin.java   |  37 ++++-
 .../hive/metastore/TestKuduMetastorePlugin.java    | 174 ++++++++++++++-------
 src/kudu/hms/hms_catalog-test.cc                   |   4 +-
 src/kudu/hms/hms_catalog.cc                        |  66 ++++----
 src/kudu/hms/hms_catalog.h                         |   5 +-
 src/kudu/hms/hms_client-test.cc                    |  13 +-
 src/kudu/hms/hms_client.cc                         |   2 +-
 src/kudu/hms/hms_client.h                          |   2 +-
 src/kudu/integration-tests/hms_itest-base.cc       |  21 +++
 src/kudu/integration-tests/hms_itest-base.h        |   5 +
 src/kudu/integration-tests/master_hms-itest.cc     |  24 ++-
 src/kudu/master/hms_notification_log_listener.cc   |  17 ++
 src/kudu/tools/kudu-tool-test.cc                   | 100 ++++++------
 src/kudu/tools/tool_action_hms.cc                  |  97 ++++++++----
 14 files changed, 379 insertions(+), 188 deletions(-)

diff --git a/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java b/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
index aaae67c..d926a9f 100644
--- a/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
+++ b/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
@@ -22,6 +22,7 @@ import java.util.Map;
 import com.google.common.annotations.VisibleForTesting;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.TableType;
 import org.apache.hadoop.hive.metastore.api.EnvironmentContext;
 import org.apache.hadoop.hive.metastore.api.MetaException;
 import org.apache.hadoop.hive.metastore.api.Table;
@@ -46,7 +47,7 @@ import org.apache.hadoop.hive.metastore.events.ListenerEvent;
  * }
  * </pre>
  *
- * The plugin enforces that Kudu table entries in the HMS always contain
+ * The plugin enforces that managed Kudu table entries in the HMS always contain
  * two properties: a Kudu table ID and the Kudu master addresses. It also
  * enforces that non-Kudu tables do not have these properties (except cases
  * when upgrading tables with legacy Kudu storage handler to be Kudu tables
@@ -69,7 +70,7 @@ public class KuduMetastorePlugin extends MetaStoreEventListener {
   @VisibleForTesting
   static final String KUDU_TABLE_ID_KEY = "kudu.table_id";
   @VisibleForTesting
-  static final String LEGACY_KUDU_TABLE_NAME = "kudu.table_name";
+  static final String KUDU_TABLE_NAME = "kudu.table_name";
   @VisibleForTesting
   static final String KUDU_MASTER_ADDRS_KEY = "kudu.master_addresses";
   @VisibleForTesting
@@ -84,6 +85,12 @@ public class KuduMetastorePlugin extends MetaStoreEventListener {
     super.onCreateTable(tableEvent);
     Table table = tableEvent.getTable();
 
+    // Only validate managed tables.
+    // Kudu only synchronizes managed tables.
+    if (!table.getTableType().equals(TableType.MANAGED_TABLE.name())) {
+      return;
+    }
+
     // Allow non-Kudu tables to be created.
     if (!isKuduTable(table)) {
       // But ensure that the new table does not contain Kudu-specific properties.
@@ -101,6 +108,13 @@ public class KuduMetastorePlugin extends MetaStoreEventListener {
   @Override
   public void onDropTable(DropTableEvent tableEvent) throws MetaException {
     super.onDropTable(tableEvent);
+    Table table = tableEvent.getTable();
+
+    // Only validate managed tables.
+    // Kudu only synchronizes managed tables.
+    if (!table.getTableType().equals(TableType.MANAGED_TABLE.name())) {
+      return;
+    }
 
     EnvironmentContext environmentContext = tableEvent.getEnvironmentContext();
     String targetTableId = environmentContext == null ? null :
@@ -114,8 +128,6 @@ public class KuduMetastorePlugin extends MetaStoreEventListener {
     // The kudu.master_event property isn't checked, because the kudu.table_id
     // property already implies this event is coming from a Kudu Master.
 
-    Table table = tableEvent.getTable();
-
     // Check that the table being dropped is a Kudu table.
     if (!isKuduTable(table)) {
       throw new MetaException("Kudu table ID does not match the non-Kudu HMS entry");
@@ -134,6 +146,23 @@ public class KuduMetastorePlugin extends MetaStoreEventListener {
     Table oldTable = tableEvent.getOldTable();
     Table newTable = tableEvent.getNewTable();
 
+    // Prevent altering the table type (managed/external) of Kudu tables.
+    // This can cause orphaned tables and the Sentry integration depends on
+    // having a managed table for each Kudu table to prevent security issues
+    // due to overlapping names with Kudu tables and tables in the HMS.
+    // Note: This doesn't prevent altering the table type for legacy tables
+    // because they should continue to work as they always have primarily for
+    // migration purposes.
+    if (isKuduTable(oldTable) && !oldTable.getTableType().equals(newTable.getTableType())) {
+      throw new MetaException("Kudu table type may not be altered");
+    }
+
+    // Only validate managed tables.
+    // Kudu only synchronizes managed tables.
+    if (!oldTable.getTableType().equals(TableType.MANAGED_TABLE.name())) {
+      return;
+    }
+
     if (isLegacyKuduTable(oldTable)) {
       if (isKuduTable(newTable)) {
         // Allow legacy tables to be upgraded to Kudu tables. Validate the upgraded
diff --git a/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java b/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
index 27ea56e..d0d3641 100644
--- a/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
+++ b/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
@@ -204,9 +204,19 @@ public class TestKuduMetastorePlugin {
     }
 
     // Check that creating a valid table is accepted.
-    Table table = newTable("table");
-    client.createTable(table, masterContext());
-    client.dropTable(table.getDbName(), table.getTableName());
+    {
+      Table table = newTable("table");
+      client.createTable(table, masterContext());
+      client.dropTable(table.getDbName(), table.getTableName());
+    }
+
+    // Check that creating a non-managed table is accepted.
+    {
+      Table table = newTable("table");
+      table.setTableType(TableType.EXTERNAL_TABLE.name());
+      client.createTable(table);
+      client.dropTable(table.getDbName(), table.getTableName());
+    }
   }
 
   @Test
@@ -237,6 +247,17 @@ public class TestKuduMetastorePlugin {
             "Kudu table entry must contain a Kudu storage handler property"));
       }
 
+      // Try to alter the Kudu table to a different type.
+      try {
+        Table alteredTable = table.deepCopy();
+        alteredTable.setTableType(TableType.EXTERNAL_TABLE.toString());
+        client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
+        fail();
+      } catch (TException e) {
+        assertTrue(e.getMessage().contains(
+            "Kudu table type may not be altered"));
+      }
+
       // Check that altering the table succeeds.
       client.alter_table(table.getDbName(), table.getTableName(), table);
 
@@ -262,7 +283,7 @@ public class TestKuduMetastorePlugin {
         alteredTable.getParameters().clear();
         alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
             KuduMetastorePlugin.LEGACY_KUDU_STORAGE_HANDLER);
-        alteredTable.putToParameters(KuduMetastorePlugin.LEGACY_KUDU_TABLE_NAME,
+        alteredTable.putToParameters(KuduMetastorePlugin.KUDU_TABLE_NAME,
             "legacy_table");
         alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY,
             "localhost");
@@ -273,52 +294,66 @@ public class TestKuduMetastorePlugin {
     }
 
     // Test altering a non-Kudu table.
-    table.getParameters().clear();
-    client.createTable(table);
-    try {
-
-      // Try to alter the table and add a Kudu table ID.
+    {
+      table.getParameters().clear();
+      client.createTable(table);
       try {
-        Table alteredTable = table.deepCopy();
-        alteredTable.putToParameters(KuduMetastorePlugin.KUDU_TABLE_ID_KEY,
-                                     UUID.randomUUID().toString());
-        client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
-        fail();
-      } catch (TException e) {
-        assertTrue(e.getMessage().contains(
-            "non-Kudu table entry must not contain a table ID property"));
-      }
 
-      // Try to alter the table and set a Kudu storage handler.
-      try {
-        Table alteredTable = table.deepCopy();
-        alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
-                                     KuduMetastorePlugin.KUDU_STORAGE_HANDLER);
-        client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
-        fail();
-      } catch (TException e) {
-        assertTrue(e.getMessage().contains(
-            "non-Kudu table entry must not contain the Kudu storage handler"));
-      }
+        // Try to alter the table and add a Kudu table ID.
+        try {
+          Table alteredTable = table.deepCopy();
+          alteredTable.putToParameters(KuduMetastorePlugin.KUDU_TABLE_ID_KEY,
+                                       UUID.randomUUID().toString());
+          client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
+          fail();
+        } catch (TException e) {
+          assertTrue(e.getMessage().contains(
+              "non-Kudu table entry must not contain a table ID property"));
+        }
+
+        // Try to alter the table and set a Kudu storage handler.
+        try {
+          Table alteredTable = table.deepCopy();
+          alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
+                                       KuduMetastorePlugin.KUDU_STORAGE_HANDLER);
+          client.alter_table(table.getDbName(), table.getTableName(), alteredTable);
+          fail();
+        } catch (TException e) {
+          assertTrue(e.getMessage().contains(
+              "non-Kudu table entry must not contain the Kudu storage handler"));
+        }
+
+        // Check that altering the table succeeds.
+        client.alter_table(table.getDbName(), table.getTableName(), table);
 
-      // Check that altering the table succeeds.
-      client.alter_table(table.getDbName(), table.getTableName(), table);
+        // Check that altering the legacy table to use the Kudu storage handler
+        // succeeds.
+        {
+          Table alteredTable = legacyTable.deepCopy();
+          alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
+                                       KuduMetastorePlugin.KUDU_STORAGE_HANDLER);
+          alteredTable.putToParameters(KuduMetastorePlugin.KUDU_TABLE_ID_KEY,
+                                       UUID.randomUUID().toString());
+          alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY,
+                                      "localhost");
+          client.alter_table(legacyTable.getDbName(), legacyTable.getTableName(),
+                             alteredTable);
+        }
+      } finally {
+        client.dropTable(table.getDbName(), table.getTableName());
+      }
+    }
 
-      // Check that altering the legacy table to use the Kudu storage handler
-      // succeeds.
-      {
-        Table alteredTable = legacyTable.deepCopy();
-        alteredTable.putToParameters(hive_metastoreConstants.META_TABLE_STORAGE,
-                                     KuduMetastorePlugin.KUDU_STORAGE_HANDLER);
-        alteredTable.putToParameters(KuduMetastorePlugin.KUDU_TABLE_ID_KEY,
-                                     UUID.randomUUID().toString());
-        alteredTable.putToParameters(KuduMetastorePlugin.KUDU_MASTER_ADDRS_KEY,
-                                    "localhost");
-        client.alter_table(legacyTable.getDbName(), legacyTable.getTableName(),
-                           alteredTable);
+    // Test altering a non-managed table is accepted.
+    {
+      table.getParameters().clear();
+      table.setTableType(TableType.EXTERNAL_TABLE.name());
+      client.createTable(table);
+      try {
+        client.alter_table(table.getDbName(), table.getTableName(), table);
+      } finally {
+        client.dropTable(table.getDbName(), table.getTableName());
       }
-    } finally {
-      client.dropTable(table.getDbName(), table.getTableName());
     }
   }
 
@@ -381,18 +416,45 @@ public class TestKuduMetastorePlugin {
                      /* ignore unknown */ false,
                      envContext);
 
+    // Test dropping a non-Kudu table with a Kudu table ID.
+    {
+      table.getParameters().clear();
+      client.createTable(table);
+      try {
+        client.dropTable(table.getDbName(), table.getTableName(),
+            /* delete data */ true,
+            /* ignore unknown */ false,
+            envContext);
+        fail();
+      } catch (TException e) {
+        assertTrue(e.getMessage().contains("Kudu table ID does not match the non-Kudu HMS entry"));
+      } finally {
+        client.dropTable(table.getDbName(), table.getTableName());
+      }
+    }
+
     // Test dropping a non-Kudu table.
-    table.getParameters().clear();
-    client.createTable(table);
-    try {
-      client.dropTable(table.getDbName(), table.getTableName(),
-                       /* delete data */ true,
-                       /* ignore unknown */ false,
-                       envContext);
-      fail();
-    } catch (TException e) {
-      assertTrue(e.getMessage().contains("Kudu table ID does not match the non-Kudu HMS entry"));
-    } finally {
+    {
+      table.getParameters().clear();
+      client.createTable(table);
+      try {
+        client.dropTable(table.getDbName(), table.getTableName(),
+            /* delete data */ true,
+            /* ignore unknown */ false,
+            envContext);
+        fail();
+      } catch (TException e) {
+        assertTrue(e.getMessage().contains("Kudu table ID does not match the non-Kudu HMS entry"));
+      } finally {
+        client.dropTable(table.getDbName(), table.getTableName());
+      }
+    }
+
+    // Test dropping a non-managed table is accepted.
+    {
+      table.getParameters().clear();
+      table.setTableType(TableType.EXTERNAL_TABLE.name());
+      client.createTable(table);
       client.dropTable(table.getDbName(), table.getTableName());
     }
   }
diff --git a/src/kudu/hms/hms_catalog-test.cc b/src/kudu/hms/hms_catalog-test.cc
index 9c57b9e..3630ac8 100644
--- a/src/kudu/hms/hms_catalog-test.cc
+++ b/src/kudu/hms/hms_catalog-test.cc
@@ -244,13 +244,13 @@ class HmsCatalogTest : public KuduTest {
     table.__set_parameters({
         make_pair(HmsClient::kStorageHandlerKey,
                   HmsClient::kLegacyKuduStorageHandler),
-        make_pair(HmsClient::kLegacyKuduTableNameKey,
+        make_pair(HmsClient::kKuduTableNameKey,
                   kudu_table_name),
         make_pair(HmsClient::kKuduMasterAddrsKey,
                   kMasterAddrs),
     });
 
-    // TODO(Hao): Remove this once HIVE-19253 is fixed.
+    // TODO(HIVE-19253): Used along with table type to indicate an external table.
     if (table_type == HmsClient::kExternalTable) {
       table.parameters[HmsClient::kExternalTableKey] = "TRUE";
     }
diff --git a/src/kudu/hms/hms_catalog.cc b/src/kudu/hms/hms_catalog.cc
index fd564ea..c46a500 100644
--- a/src/kudu/hms/hms_catalog.cc
+++ b/src/kudu/hms/hms_catalog.cc
@@ -133,9 +133,10 @@ void HmsCatalog::Stop() {
 Status HmsCatalog::CreateTable(const string& id,
                                const string& name,
                                optional<const string&> owner,
-                               const Schema& schema) {
+                               const Schema& schema,
+                               const string& table_type) {
   hive::Table table;
-  RETURN_NOT_OK(PopulateTable(id, name, owner, schema, master_addresses_, &table));
+  RETURN_NOT_OK(PopulateTable(id, name, owner, schema, master_addresses_, table_type, &table));
   return ha_client_.Execute([&] (HmsClient* client) {
       return client->CreateTable(table, EnvironmentContext());
   });
@@ -172,8 +173,15 @@ Status HmsCatalog::UpgradeLegacyImpalaTable(const string& id,
       return Status::IllegalState("non-legacy table cannot be upgraded");
     }
 
-    RETURN_NOT_OK(PopulateTable(id, Substitute("$0.$1", db_name, tb_name),
-                                none, schema, master_addresses_, &table));
+    // If this is an external table, only upgrade the storage handler.
+    if (table.tableType == HmsClient::kExternalTable) {
+      table.parameters[HmsClient::kStorageHandlerKey] = HmsClient::kKuduStorageHandler;
+    } else if (table.tableType == HmsClient::kManagedTable) {
+      RETURN_NOT_OK(PopulateTable(id, Substitute("$0.$1", db_name, tb_name),
+                                  table.owner, schema, master_addresses_, table.tableType, &table));
+    } else {
+      return Status::IllegalState(Substitute("Unsupported table type $0", table.tableType));
+    }
     return client->AlterTable(db_name, tb_name, table, EnvironmentContext());
   });
 }
@@ -190,18 +198,12 @@ Status HmsCatalog::DowngradeToLegacyImpalaTable(const string& name) {
         HmsClient::kKuduStorageHandler) {
       return Status::IllegalState("non-Kudu table cannot be downgraded");
     }
-
-    // Add the legacy Kudu-specific parameters. And set it to
-    // external table type.
-    table.tableType = HmsClient::kExternalTable;
-    table.parameters[HmsClient::kLegacyKuduTableNameKey] = name;
-    table.parameters[HmsClient::kKuduMasterAddrsKey] = master_addresses_;
+    // Downgrade the storage handler.
     table.parameters[HmsClient::kStorageHandlerKey] = HmsClient::kLegacyKuduStorageHandler;
-    table.parameters[HmsClient::kExternalTableKey] = "TRUE";
-
-    // Remove the Kudu-specific field 'kudu.table_id'.
-    EraseKeyReturnValuePtr(&table.parameters, HmsClient::kKuduTableIdKey);
-
+    if (table.tableType == HmsClient::kManagedTable) {
+      // Remove the Kudu-specific field 'kudu.table_id'.
+      EraseKeyReturnValuePtr(&table.parameters, HmsClient::kKuduTableIdKey);
+    }
     return client->AlterTable(table.dbName, table.tableName, table, EnvironmentContext());
   });
 }
@@ -271,7 +273,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, none, schema, master_addresses_, &table));
+      RETURN_NOT_OK(PopulateTable(id, new_name, table.owner, schema, master_addresses_,
+          table.tableType, &table));
       return client->AlterTable(hms_database.ToString(), hms_table.ToString(),
                                 table, EnvironmentContext());
   });
@@ -330,6 +333,7 @@ Status HmsCatalog::PopulateTable(const string& id,
                                  const optional<const string&>& owner,
                                  const Schema& schema,
                                  const string& master_addresses,
+                                 const string& table_type,
                                  hive::Table* table) {
   Slice hms_database_name;
   Slice hms_table_name;
@@ -340,22 +344,28 @@ Status HmsCatalog::PopulateTable(const string& id,
     table->owner = *owner;
   }
 
+  // TODO(HIVE-21640): Fix the issue described below and in HIVE-21640
+  // Setting the table type to managed means the table's (HD)FS directory will
+  // be deleted when the table is dropped. Deleting the directory is
+  // unnecessary, and causes a race in the HMS between concurrent DROP TABLE and
+  // CREATE TABLE operations on existing tables.
+  table->tableType = table_type;
+
+  // TODO(HIVE-19253): Used along with table type to indicate an external table.
+  if (table_type == HmsClient::kExternalTable) {
+    table->parameters[HmsClient::kExternalTableKey] = "TRUE";
+  // Only set the table id on managed tables and ensure the
+  // kExternalTableKey property is unset.
+  } else if (table_type == HmsClient::kManagedTable) {
+    table->parameters[HmsClient::kKuduTableIdKey] = id;
+    EraseKeyReturnValuePtr(&table->parameters, HmsClient::kExternalTableKey);
+  }
+
   // Add the Kudu-specific parameters. This intentionally avoids overwriting
   // other parameters.
-  table->parameters[HmsClient::kKuduTableIdKey] = id;
+  table->parameters[HmsClient::kKuduTableNameKey] = name;
   table->parameters[HmsClient::kKuduMasterAddrsKey] = master_addresses;
   table->parameters[HmsClient::kStorageHandlerKey] = HmsClient::kKuduStorageHandler;
-  // Workaround for HIVE-19253.
-  table->parameters[HmsClient::kExternalTableKey] = "TRUE";
-
-  // Set the table type to external so that the table's (HD)FS directory will
-  // not be deleted when the table is dropped. Deleting the directory is
-  // unnecessary, and causes a race in the HMS between concurrent DROP TABLE and
-  // CREATE TABLE operations on existing tables.
-  table->tableType = HmsClient::kExternalTable;
-
-  // Remove the deprecated Kudu-specific field 'kudu.table_name'.
-  EraseKeyReturnValuePtr(&table->parameters, HmsClient::kLegacyKuduTableNameKey);
 
   // Overwrite the entire set of columns.
   vector<hive::FieldSchema> fields;
diff --git a/src/kudu/hms/hms_catalog.h b/src/kudu/hms/hms_catalog.h
index d0b2a83..22b2058 100644
--- a/src/kudu/hms/hms_catalog.h
+++ b/src/kudu/hms/hms_catalog.h
@@ -64,7 +64,9 @@ class HmsCatalog {
   Status CreateTable(const std::string& id,
                      const std::string& name,
                      boost::optional<const std::string&> owner,
-                     const Schema& schema) WARN_UNUSED_RESULT;
+                     const Schema& schema,
+                     const std::string& table_type = hms::HmsClient::kManagedTable)
+                     WARN_UNUSED_RESULT;
 
   // Drops a table entry from the HMS.
   //
@@ -141,6 +143,7 @@ class HmsCatalog {
                               const boost::optional<const std::string&>& owner,
                               const Schema& schema,
                               const std::string& master_addresses,
+                              const std::string& table_type,
                               hive::Table* table) WARN_UNUSED_RESULT;
 
   // Validates and canonicalizes the provided table name according to HMS rules.
diff --git a/src/kudu/hms/hms_client-test.cc b/src/kudu/hms/hms_client-test.cc
index 5424c63..ca64c6b 100644
--- a/src/kudu/hms/hms_client-test.cc
+++ b/src/kudu/hms/hms_client-test.cc
@@ -66,13 +66,12 @@ class HmsClientTest : public KuduTest,
     hive::Table table;
     table.dbName = database_name;
     table.tableName = table_name;
-    table.tableType = HmsClient::kExternalTable;
-
+    table.tableType = HmsClient::kManagedTable;
     table.__set_parameters({
         make_pair(HmsClient::kKuduTableIdKey, table_id),
+        make_pair(HmsClient::kKuduTableNameKey, table_name),
         make_pair(HmsClient::kKuduMasterAddrsKey, string("TODO")),
-        make_pair(HmsClient::kStorageHandlerKey, HmsClient::kKuduStorageHandler),
-        make_pair(HmsClient::kExternalTableKey, "TRUE")
+        make_pair(HmsClient::kStorageHandlerKey, HmsClient::kKuduStorageHandler)
     });
 
     hive::EnvironmentContext env_ctx;
@@ -175,7 +174,7 @@ TEST_P(HmsClientTest, TestHmsOperations) {
   EXPECT_EQ(table_name, my_table.tableName);
   EXPECT_EQ(table_id, my_table.parameters[HmsClient::kKuduTableIdKey]);
   EXPECT_EQ(HmsClient::kKuduStorageHandler, my_table.parameters[HmsClient::kStorageHandlerKey]);
-  EXPECT_EQ(HmsClient::kExternalTable, my_table.tableType);
+  EXPECT_EQ(HmsClient::kManagedTable, my_table.tableType);
 
   string new_table_name = "my_altered_table";
 
@@ -200,7 +199,7 @@ TEST_P(HmsClientTest, TestHmsOperations) {
   EXPECT_EQ(table_id, renamed_table.parameters[HmsClient::kKuduTableIdKey]);
   EXPECT_EQ(HmsClient::kKuduStorageHandler,
             renamed_table.parameters[HmsClient::kStorageHandlerKey]);
-  EXPECT_EQ(HmsClient::kExternalTable, renamed_table.tableType);
+  EXPECT_EQ(HmsClient::kManagedTable, renamed_table.tableType);
 
   // Create a table with an uppercase name.
   string uppercase_table_name = "my_UPPERCASE_Table";
@@ -321,7 +320,7 @@ TEST_P(HmsClientTest, TestLargeObjects) {
   hive::Table table;
   table.dbName = database_name;
   table.tableName = table_name;
-  table.tableType = HmsClient::kExternalTable;
+  table.tableType = HmsClient::kManagedTable;
   hive::FieldSchema partition_key;
   partition_key.name = "c1";
   partition_key.type = "int";
diff --git a/src/kudu/hms/hms_client.cc b/src/kudu/hms/hms_client.cc
index 3482163..679fd6a 100644
--- a/src/kudu/hms/hms_client.cc
+++ b/src/kudu/hms/hms_client.cc
@@ -94,9 +94,9 @@ namespace hms {
 
 const char* const HmsClient::kLegacyKuduStorageHandler =
   "com.cloudera.kudu.hive.KuduStorageHandler";
-const char* const HmsClient::kLegacyKuduTableNameKey = "kudu.table_name";
 const char* const HmsClient::kLegacyTablePrefix = "impala::";
 const char* const HmsClient::kKuduTableIdKey = "kudu.table_id";
+const char* const HmsClient::kKuduTableNameKey = "kudu.table_name";
 const char* const HmsClient::kKuduMasterAddrsKey = "kudu.master_addresses";
 const char* const HmsClient::kKuduMasterEventKey = "kudu.master_event";
 const char* const HmsClient::kKuduStorageHandler = "org.apache.kudu.hive.KuduStorageHandler";
diff --git a/src/kudu/hms/hms_client.h b/src/kudu/hms/hms_client.h
index da8cf7b..2effe11 100644
--- a/src/kudu/hms/hms_client.h
+++ b/src/kudu/hms/hms_client.h
@@ -58,9 +58,9 @@ class HmsClient {
 
   static const char* const kExternalTableKey;
   static const char* const kLegacyKuduStorageHandler;
-  static const char* const kLegacyKuduTableNameKey;
   static const char* const kLegacyTablePrefix;
   static const char* const kKuduTableIdKey;
+  static const char* const kKuduTableNameKey;
   static const char* const kKuduMasterAddrsKey;
   static const char* const kKuduMasterEventKey;;
   static const char* const kStorageHandlerKey;
diff --git a/src/kudu/integration-tests/hms_itest-base.cc b/src/kudu/integration-tests/hms_itest-base.cc
index 4a29744..547c2ab 100644
--- a/src/kudu/integration-tests/hms_itest-base.cc
+++ b/src/kudu/integration-tests/hms_itest-base.cc
@@ -24,6 +24,7 @@
 #include <utility>
 #include <vector>
 
+#include <boost/algorithm/string/predicate.hpp>
 #include <gtest/gtest.h>
 
 #include "kudu/client/client.h"
@@ -111,6 +112,20 @@ Status HmsITestBase::CreateKuduTable(const string& database_name,
       .Create();
 }
 
+Status HmsITestBase::CreateHmsTable(const string& database_name,
+                                    const string& table_name,
+                                    const string& table_type) {
+  hive::Table hms_table;
+  hms_table.dbName = database_name;
+  hms_table.tableName = table_name;
+  hms_table.tableType = table_type;
+  // TODO(HIVE-19253): Used along with table type to indicate an external table.
+  if (table_type == HmsClient::kExternalTable) {
+    hms_table.parameters[HmsClient::kExternalTableKey] = "TRUE";
+  }
+  return hms_client_->CreateTable(hms_table);
+}
+
 Status HmsITestBase::RenameHmsTable(const string& database_name,
                                     const string& old_table_name,
                                     const string& new_table_name) {
@@ -121,6 +136,8 @@ Status HmsITestBase::RenameHmsTable(const string& database_name,
   hive::Table table;
   RETURN_NOT_OK(hms_client_->GetTable(database_name, old_table_name, &table));
   table.tableName = new_table_name;
+  table.parameters[hms::HmsClient::kKuduTableNameKey] =
+      Substitute("$0.$1", database_name, new_table_name);
   return hms_client_->AlterTable(database_name, old_table_name, table);
 }
 
@@ -148,6 +165,8 @@ void HmsITestBase::CheckTable(const string& database_name,
   hive::Table hms_table;
   ASSERT_OK(hms_client_->GetTable(database_name, table_name, &hms_table));
 
+  ASSERT_EQ(hms::HmsClient::kManagedTable, hms_table.tableType);
+
   string username;
   if (user) {
     username = *user;
@@ -163,6 +182,8 @@ void HmsITestBase::CheckTable(const string& database_name,
     ASSERT_EQ(schema.Column(idx).comment(), hms_table.sd.cols[idx].comment);
   }
   ASSERT_EQ(table->id(), hms_table.parameters[hms::HmsClient::kKuduTableIdKey]);
+  ASSERT_TRUE(boost::iequals(table->name(),
+      hms_table.parameters[hms::HmsClient::kKuduTableNameKey]));
   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 50c814c..b3fb011 100644
--- a/src/kudu/integration-tests/hms_itest-base.h
+++ b/src/kudu/integration-tests/hms_itest-base.h
@@ -44,6 +44,11 @@ class HmsITestBase : public ExternalMiniClusterITestBase {
                          const std::string& table_name,
                          MonoDelta timeout = {}) WARN_UNUSED_RESULT;
 
+  // Creates a table in the HMS catalog.
+  Status CreateHmsTable(const std::string& database_name,
+                        const std::string& table_name,
+                        const std::string& table_type = hms::HmsClient::kManagedTable);
+
   // Renames a table entry in the HMS catalog.
   Status RenameHmsTable(const std::string& database_name,
                         const std::string& old_table_name,
diff --git a/src/kudu/integration-tests/master_hms-itest.cc b/src/kudu/integration-tests/master_hms-itest.cc
index 0e680b4..6704fec 100644
--- a/src/kudu/integration-tests/master_hms-itest.cc
+++ b/src/kudu/integration-tests/master_hms-itest.cc
@@ -108,10 +108,7 @@ TEST_F(MasterHmsTest, TestCreateTable) {
   ASSERT_OK(CreateDatabase(hms_database_name));
 
   // Create a table entry with the name.
-  hive::Table hms_table;
-  hms_table.dbName = hms_database_name;
-  hms_table.tableName = hms_table_name;
-  ASSERT_OK(hms_client_->CreateTable(hms_table));
+  ASSERT_OK(CreateHmsTable(hms_database_name, hms_table_name));
 
   // Attempt to create a Kudu table with the same name.
   s = CreateKuduTable(hms_database_name, hms_table_name);
@@ -128,6 +125,13 @@ TEST_F(MasterHmsTest, TestCreateTable) {
   ASSERT_OK(CreateKuduTable(hms_database_name, hms_table_name));
   NO_FATALS(CheckTable(hms_database_name, hms_table_name, /*user=*/none));
 
+  // Create an external table in the HMS and validate it's not created Kudu side.
+  const char* external_table_name = "externalTable";
+  ASSERT_OK(CreateHmsTable(hms_database_name, external_table_name, HmsClient::kExternalTable));
+  shared_ptr<KuduTable> table;
+  s = client_->OpenTable(Substitute("$0.$1", hms_database_name, external_table_name), &table);
+  ASSERT_TRUE(s.IsNotFound()) << s.ToString();
+
   // Shutdown the HMS and try to create a table.
   ASSERT_OK(StopHms());
 
@@ -150,10 +154,7 @@ TEST_F(MasterHmsTest, TestRenameTable) {
   NO_FATALS(CheckTable("db", "a", /*user=*/none));
 
   // Create a non-Kudu ('external') HMS table entry.
-  hive::Table external_table;
-  external_table.dbName = "db";
-  external_table.tableName = "b";
-  ASSERT_OK(hms_client_->CreateTable(external_table));
+  ASSERT_OK(CreateHmsTable("db", "b", HmsClient::kExternalTable));
 
   // Attempt to rename the Kudu table to the same name.
   unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer("db.a"));
@@ -212,6 +213,9 @@ TEST_F(MasterHmsTest, TestRenameTable) {
   std::sort(tables.begin(), tables.end());
   ASSERT_EQ(tables, vector<string>({ "b", "d" })) << tables;
 
+  // Rename the external table through the HMS and ensure Kudu allows it.
+  ASSERT_OK(RenameHmsTable("db", "b", "e"));
+
   // Regression test for HIVE-19569
   // If HIVE-19569 is in effect the rename across databases will result in DROP
   // TABLE and CREATE TABLE events, which will cause the notification log
@@ -319,6 +323,10 @@ TEST_F(MasterHmsTest, TestDeleteTable) {
   s = client_->OpenTable(Substitute("$0.$1", "default", "d"), &table);
   ASSERT_TRUE(s.IsNotFound()) << s.ToString();
   ASSERT_OK(hms_client_->GetTable("default", "d", &hms_table_d));
+
+  // Create and drop a non-Kudu ('external') HMS table entry and ensure Kudu allows it.
+  ASSERT_OK(CreateHmsTable("default", "externalTable", HmsClient::kExternalTable));
+  ASSERT_OK(hms_client_->DropTable("default", "externalTable"));
 }
 
 TEST_F(MasterHmsTest, TestNotificationLogListener) {
diff --git a/src/kudu/master/hms_notification_log_listener.cc b/src/kudu/master/hms_notification_log_listener.cc
index 2237a4b..63a5eb5 100644
--- a/src/kudu/master/hms_notification_log_listener.cc
+++ b/src/kudu/master/hms_notification_log_listener.cc
@@ -333,10 +333,19 @@ Status HmsNotificationLogListenerTask::HandleAlterTableEvent(const hive::Notific
   hive::Table before_table;
   RETURN_NOT_OK(DeserializeTable(event, message, "tableObjBeforeJson", &before_table));
 
+  if (before_table.tableType != hms::HmsClient::kManagedTable) {
+    // Not a managed table; skip it.
+    VLOG(2) << Substitute("Ignoring alter event for table $0 of type $1",
+                          before_table.tableName, before_table.tableType);
+    return Status::OK();
+  }
+
   const string* storage_handler =
       FindOrNull(before_table.parameters, hms::HmsClient::kStorageHandlerKey);
   if (!storage_handler || *storage_handler != hms::HmsClient::kKuduStorageHandler) {
     // Not a Kudu table; skip it.
+    VLOG(2) << Substitute("Ignoring alter event for non-Kudu table $0",
+                          before_table.tableName);
     return Status::OK();
   }
 
@@ -384,9 +393,17 @@ Status HmsNotificationLogListenerTask::HandleDropTableEvent(const hive::Notifica
   hive::Table table;
   RETURN_NOT_OK(DeserializeTable(event, message, "tableObjJson", &table));
 
+  if (table.tableType != hms::HmsClient::kManagedTable) {
+    // Not a managed table; skip it.
+    VLOG(2) << Substitute("Ignoring drop event for table $0 of type $1",
+                          table.tableName, table.tableType);
+    return Status::OK();
+  }
+
   const string* storage_handler = FindOrNull(table.parameters, hms::HmsClient::kStorageHandlerKey);
   if (!storage_handler || *storage_handler != hms::HmsClient::kKuduStorageHandler) {
     // Not a Kudu table; skip it.
+    VLOG(2) << Substitute("Ignoring drop event for non-Kudu table $0", table.tableName);
     return Status::OK();
   }
 
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index 9d445fb..422a719 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -34,6 +34,7 @@
 #include <utility>
 #include <vector>
 
+#include <boost/algorithm/string/predicate.hpp>
 #include <boost/optional/optional.hpp>
 #include <gflags/gflags_declare.h>
 #include <glog/logging.h>
@@ -3227,11 +3228,11 @@ Status CreateLegacyHmsTable(HmsClient* client,
 
   table.__set_parameters({
       make_pair(HmsClient::kStorageHandlerKey, HmsClient::kLegacyKuduStorageHandler),
-      make_pair(HmsClient::kLegacyKuduTableNameKey, kudu_table_name),
+      make_pair(HmsClient::kKuduTableNameKey, kudu_table_name),
       make_pair(HmsClient::kKuduMasterAddrsKey, kudu_master_addrs),
   });
 
-  // TODO(Hao): Remove this once HIVE-19253 is fixed.
+  // TODO(HIVE-19253): Used along with table type to indicate an external table.
   if (table_type == HmsClient::kExternalTable) {
     table.parameters[HmsClient::kExternalTableKey] = "TRUE";
   }
@@ -3250,17 +3251,21 @@ Status CreateHmsTable(HmsClient* client,
   table.tableName = table_name;
   table.tableType = table_type;
 
-  table.__set_parameters({
-      make_pair(HmsClient::kStorageHandlerKey, HmsClient::kKuduStorageHandler),
-      make_pair(HmsClient::kKuduTableIdKey, table_id),
-      make_pair(HmsClient::kKuduMasterAddrsKey, master_addresses),
-  });
-
-  // TODO(Hao): Remove this once HIVE-19253 is fixed.
+  // TODO(HIVE-19253): Used along with table type to indicate an external table.
   if (table_type == HmsClient::kExternalTable) {
     table.parameters[HmsClient::kExternalTableKey] = "TRUE";
   }
 
+  // Only set the table id on managed tables.
+  if (table_type == HmsClient::kManagedTable) {
+    table.parameters[HmsClient::kKuduTableIdKey] = table_id;
+  }
+
+  table.parameters[HmsClient::kKuduTableNameKey] =
+      Substitute("$0.$1", database_name, table_name);
+  table.parameters[HmsClient::kKuduMasterAddrsKey] = master_addresses;
+  table.parameters[HmsClient::kStorageHandlerKey] = HmsClient::kKuduStorageHandler;
+
   hive::EnvironmentContext env_ctx;
   env_ctx.__set_properties({ make_pair(HmsClient::kKuduMasterEventKey, "true") });
   return client->CreateTable(table, env_ctx);
@@ -3296,12 +3301,22 @@ void ValidateHmsEntries(HmsClient* hms_client,
                         const string& master_addr) {
   hive::Table hms_table;
   ASSERT_OK(hms_client->GetTable(database_name, table_name, &hms_table));
-  shared_ptr<KuduTable> kudu_table;
-  ASSERT_OK(kudu_client->OpenTable(Substitute("$0.$1", database_name, table_name), &kudu_table));
+
   ASSERT_EQ(hms_table.parameters[HmsClient::kStorageHandlerKey], HmsClient::kKuduStorageHandler);
-  ASSERT_EQ(hms_table.parameters[HmsClient::kKuduTableIdKey], kudu_table->id());
   ASSERT_EQ(hms_table.parameters[HmsClient::kKuduMasterAddrsKey], master_addr);
-  ASSERT_TRUE(!ContainsKey(hms_table.parameters, HmsClient::kLegacyKuduTableNameKey));
+
+  if (hms_table.tableType == HmsClient::kManagedTable) {
+    shared_ptr<KuduTable> kudu_table;
+    ASSERT_OK(kudu_client->OpenTable(Substitute("$0.$1", database_name, table_name), &kudu_table));
+    ASSERT_TRUE(boost::iequals(kudu_table->name(),
+                               hms_table.parameters[hms::HmsClient::kKuduTableNameKey]));
+    ASSERT_EQ(kudu_table->id(), hms_table.parameters[HmsClient::kKuduTableIdKey]);
+  }
+
+  if (hms_table.tableType == HmsClient::kExternalTable) {
+    ASSERT_TRUE(ContainsKey(hms_table.parameters, HmsClient::kKuduTableNameKey));
+    ASSERT_FALSE(ContainsKey(hms_table.parameters, HmsClient::kKuduTableIdKey));
+  }
 }
 
 TEST_P(ToolTestKerberosParameterized, TestHmsDowngrade) {
@@ -3422,9 +3437,6 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
   ASSERT_OK(hms_catalog.CreateTable(
         "orphan-hms-table-id", "default.orphan_hms_table", kUsername,
         SchemaBuilder().Build()));
-  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "orphan_hms_table_legacy_external",
-        "default.orphan_hms_table_legacy_external",
-        master_addr, HmsClient::kExternalTable, kUsername));
   ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "orphan_hms_table_legacy_managed",
         "impala::default.orphan_hms_table_legacy_managed",
         master_addr, HmsClient::kManagedTable, kUsername));
@@ -3432,19 +3444,16 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
   // Test case: orphan table in Kudu.
   ASSERT_OK(CreateKuduTable(kudu_client, "default.kudu_orphan"));
 
-  // Test case: legacy external table.
-  shared_ptr<KuduTable> legacy_external;
-  ASSERT_OK(CreateKuduTable(kudu_client, "default.legacy_external"));
-  ASSERT_OK(kudu_client->OpenTable("default.legacy_external", &legacy_external));
-  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_external",
-        "default.legacy_external", master_addr, HmsClient::kExternalTable, kUsername));
-
   // Test case: legacy managed table.
   shared_ptr<KuduTable> legacy_managed;
   ASSERT_OK(CreateKuduTable(kudu_client, "impala::default.legacy_managed"));
   ASSERT_OK(kudu_client->OpenTable("impala::default.legacy_managed", &legacy_managed));
   ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_managed",
-        "impala::default.legacy_managed", master_addr, HmsClient::kManagedTable, kUsername));
+      "impala::default.legacy_managed", master_addr, HmsClient::kManagedTable, kUsername));
+
+  // Test case: legacy external table (pointed at the legacy managed table).
+  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_external",
+      "impala::default.legacy_managed", master_addr, HmsClient::kExternalTable, kUsername));
 
   // Test case: legacy managed table with no owner.
   shared_ptr<KuduTable> legacy_no_owner;
@@ -3453,14 +3462,14 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
   ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_no_owner",
         "impala::default.legacy_no_owner", master_addr, HmsClient::kManagedTable, boost::none));
 
-  // Test case: legacy external table with a Hive-incompatible name (no database).
-  shared_ptr<KuduTable> legacy_external_hive_incompatible_name;
-  ASSERT_OK(CreateKuduTable(kudu_client, "legacy_external_hive_incompatible_name"));
-  ASSERT_OK(kudu_client->OpenTable("legacy_external_hive_incompatible_name",
-        &legacy_external_hive_incompatible_name));
-  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_external_hive_incompatible_name",
-        "legacy_external_hive_incompatible_name", master_addr,
-        HmsClient::kExternalTable, kUsername));
+  // Test case: legacy managed table with a Hive-incompatible name (no database).
+  shared_ptr<KuduTable> legacy_hive_incompatible_name;
+  ASSERT_OK(CreateKuduTable(kudu_client, "legacy_hive_incompatible_name"));
+  ASSERT_OK(kudu_client->OpenTable("legacy_hive_incompatible_name",
+        &legacy_hive_incompatible_name));
+  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_hive_incompatible_name",
+        "legacy_hive_incompatible_name", master_addr,
+        HmsClient::kManagedTable, kUsername));
 
   // Test case: Kudu table in non-default database.
   hive::Database db;
@@ -3483,13 +3492,12 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
     "default.inconsistent_name",
     "default.inconsistent_master_addrs",
     "default.orphan_hms_table",
-    "default.orphan_hms_table_legacy_external",
     "default.orphan_hms_table_legacy_managed",
     "default.kudu_orphan",
-    "default.legacy_external",
     "default.legacy_managed",
     "default.legacy_no_owner",
-    "legacy_external_hive_incompatible_name",
+    "legacy_external",
+    "legacy_hive_incompatible_name",
     "my_db.table",
   };
 
@@ -3531,13 +3539,12 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
         Substitute("hms fix $0 --dryrun --drop_orphan_hms_tables", master_addr)));
   NO_FATALS(check());
 
-  // Drop orphan tables.
+  // Drop orphan hms tables.
   NO_FATALS(RunActionStdoutNone(
         Substitute("hms fix $0 --drop_orphan_hms_tables --nocreate_missing_hms_tables "
                    "--noupgrade_hms_tables --nofix_inconsistent_tables", master_addr)));
   make_consistent({
     "default.orphan_hms_table",
-    "default.orphan_hms_table_legacy_external",
     "default.orphan_hms_table_legacy_managed",
   });
   NO_FATALS(check());
@@ -3555,10 +3562,10 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
   NO_FATALS(RunActionStdoutNone(
         Substitute("hms fix $0 --nofix_inconsistent_tables", master_addr)));
   make_consistent({
-    "default.legacy_external",
     "default.legacy_managed",
+    "legacy_external",
     "default.legacy_no_owner",
-    "legacy_external_hive_incompatible_name",
+    "legacy_hive_incompatible_name",
   });
   NO_FATALS(check());
 
@@ -3581,9 +3588,9 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
     "inconsistent_name_hms",
     "inconsistent_master_addrs",
     "kudu_orphan",
-    "legacy_external",
     "legacy_managed",
-    "legacy_external_hive_incompatible_name",
+    "legacy_external",
+    "legacy_hive_incompatible_name",
   }) {
     NO_FATALS(ValidateHmsEntries(&hms_client, kudu_client, "default", table, master_addr));
   }
@@ -3600,8 +3607,7 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
     "default.inconsistent_name_hms",
     "default.inconsistent_schema",
     "default.kudu_orphan",
-    "default.legacy_external",
-    "default.legacy_external_hive_incompatible_name",
+    "default.legacy_hive_incompatible_name",
     "default.legacy_managed",
     "default.legacy_no_owner",
     "default.uppercase",
@@ -3610,7 +3616,6 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
 
   // Check that table ownership is preserved in upgraded legacy tables.
   for (auto p : vector<pair<string, string>>({
-        make_pair("legacy_external", kUsername),
         make_pair("legacy_managed", kUsername),
         make_pair("legacy_no_owner", ""),
   })) {
@@ -3621,6 +3626,8 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndAutomaticFixHmsMetadata) {
 }
 
 // Test HMS inconsistencies that must be manually fixed.
+// TODO(ghenke): Add test case for external table using the same name as
+//  an existing Kudu table.
 TEST_P(ToolTestKerberosParameterized, TestCheckAndManualFixHmsMetadata) {
   string kUsername = "alice";
   ExternalMiniClusterOptions opts;
@@ -3657,9 +3664,6 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndManualFixHmsMetadata) {
   ASSERT_OK(hms_catalog.CreateTable(
         duplicate_hms_tables->id(), "default.duplicate_hms_tables_2", kUsername,
         KuduSchema::ToSchema(duplicate_hms_tables->schema())));
-  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "duplicate_hms_tables_3",
-        "default.duplicate_hms_tables",
-        master_addr, HmsClient::kExternalTable, kUsername));
 
   // Test case: Kudu tables Hive-incompatible names.
   ASSERT_OK(CreateKuduTable(kudu_client, "default.hive-incompatible-name"));
@@ -3689,7 +3693,6 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndManualFixHmsMetadata) {
     for (const string& table : vector<string>({
       "duplicate_hms_tables",
       "duplicate_hms_tables_2",
-      "duplicate_hms_tables_3",
       "default.hive-incompatible-name",
       "no_database",
       "non_existent_database.table",
@@ -3716,7 +3719,6 @@ TEST_P(ToolTestKerberosParameterized, TestCheckAndManualFixHmsMetadata) {
 
   // Manually drop the duplicate HMS entries.
   ASSERT_OK(hms_catalog.DropTable(duplicate_hms_tables->id(), "default.duplicate_hms_tables_2"));
-  ASSERT_OK(hms_catalog.DropLegacyTable("default.duplicate_hms_tables_3"));
 
   // Rename the incompatible names.
   NO_FATALS(RunActionStdoutNone(Substitute(
diff --git a/src/kudu/tools/tool_action_hms.cc b/src/kudu/tools/tool_action_hms.cc
index be04332..5ca4d9d 100644
--- a/src/kudu/tools/tool_action_hms.cc
+++ b/src/kudu/tools/tool_action_hms.cc
@@ -66,10 +66,6 @@ DEFINE_bool(upgrade_hms_tables, true,
 
 namespace kudu {
 
-namespace server {
-class GetFlagsResponsePB_Flag;
-} // namespace server
-
 namespace tools {
 
 using client::KuduClient;
@@ -80,7 +76,6 @@ using client::KuduTableAlterer;
 using client::sp::shared_ptr;
 using hms::HmsCatalog;
 using hms::HmsClient;
-using server::GetFlagsResponsePB_Flag;
 using std::cout;
 using std::endl;
 using std::make_pair;
@@ -166,8 +161,9 @@ bool IsSynced(const string& master_addresses,
               const hive::Table& hms_table) {
   auto schema = KuduSchema::ToSchema(kudu_table.schema());
   hive::Table hms_table_copy(hms_table);
-  Status s = HmsCatalog::PopulateTable(kudu_table.id(), kudu_table.name(), boost::none,
-                                       schema, master_addresses, &hms_table_copy);
+  // TODO(ghenke): Get the owner from Kudu?
+  Status s = HmsCatalog::PopulateTable(kudu_table.id(), kudu_table.name(), hms_table.owner, schema,
+                                       master_addresses, hms_table.tableType, &hms_table_copy);
   return s.ok() && hms_table_copy == hms_table;
 }
 
@@ -200,8 +196,9 @@ Status PrintTables(const string& master_addrs,
       "Kudu master addresses",
       "HMS database",
       "HMS table",
+      "HMS table type",
       Substitute("HMS $0", HmsClient::kStorageHandlerKey),
-      Substitute("HMS $0", HmsClient::kLegacyKuduTableNameKey),
+      Substitute("HMS $0", HmsClient::kKuduTableNameKey),
       Substitute("HMS $0", HmsClient::kKuduTableIdKey),
       Substitute("HMS $0", HmsClient::kKuduMasterAddrsKey),
   });
@@ -219,12 +216,13 @@ Status PrintTables(const string& master_addrs,
       hive::Table& hms_table = *pair.second;
       row.emplace_back(hms_table.dbName);
       row.emplace_back(hms_table.tableName);
+      row.emplace_back(hms_table.tableType);
       row.emplace_back(hms_table.parameters[HmsClient::kStorageHandlerKey]);
-      row.emplace_back(hms_table.parameters[HmsClient::kLegacyKuduTableNameKey]);
+      row.emplace_back(hms_table.parameters[HmsClient::kKuduTableNameKey]);
       row.emplace_back(hms_table.parameters[HmsClient::kKuduTableIdKey]);
       row.emplace_back(hms_table.parameters[HmsClient::kKuduMasterAddrsKey]);
     } else {
-      row.resize(9);
+      row.resize(10);
     }
     table.AddRow(std::move(row));
   }
@@ -298,29 +296,46 @@ Status AnalyzeCatalogs(const string& master_addrs,
 
   // Step 2: retrieve all Kudu table entries in the HMS, filter all orphaned
   // entries which reference non-existent Kudu tables, and group the rest by
-  // table ID.
-  vector<hive::Table> orphan_tables;
-  unordered_map<string, vector<hive::Table>> hms_tables_by_id;
+  // table type and table ID.
+  vector<hive::Table> orphan_hms_tables;
+  unordered_map<string, vector<hive::Table>> managed_hms_tables_by_id;
+  unordered_map<string, vector<hive::Table>> external_hms_tables_by_id;
   {
     vector<hive::Table> hms_tables;
     RETURN_NOT_OK(hms_catalog->GetKuduTables(&hms_tables));
     for (hive::Table& hms_table : hms_tables) {
-      const string& storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
-      if (storage_handler == HmsClient::kKuduStorageHandler) {
-        const string& hms_table_id = hms_table.parameters[HmsClient::kKuduTableIdKey];
-        shared_ptr<KuduTable>* kudu_table = FindOrNull(kudu_tables_by_id, hms_table_id);
+      const string &storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
+      // If this is a legacy table or external table, lookup the Kudu table by name.
+      if (storage_handler == HmsClient::kLegacyKuduStorageHandler ||
+          hms_table.tableType == HmsClient::kExternalTable) {
+        const string& hms_table_name = hms_table.parameters[HmsClient::kKuduTableNameKey];
+        shared_ptr<KuduTable>* kudu_table = FindOrNull(kudu_tables_by_name,
+                                                       hms_table_name);
         if (kudu_table) {
-          hms_tables_by_id[(*kudu_table)->id()].emplace_back(std::move(hms_table));
+          if (hms_table.tableType == HmsClient::kExternalTable) {
+            external_hms_tables_by_id[(*kudu_table)->id()].emplace_back(
+                std::move(hms_table));
+          } else {
+            managed_hms_tables_by_id[(*kudu_table)->id()].emplace_back(
+                std::move(hms_table));
+          }
         } else {
-          orphan_tables.emplace_back(std::move(hms_table));
+          // External tables don't always point at valid Kudu tables.
+          if (hms_table.tableType != HmsClient::kExternalTable) {
+            orphan_hms_tables.emplace_back(std::move(hms_table));
+          }
         }
-      } else if (storage_handler == HmsClient::kLegacyKuduStorageHandler) {
-        const string& hms_table_name = hms_table.parameters[HmsClient::kLegacyKuduTableNameKey];
-        shared_ptr<KuduTable>* kudu_table = FindOrNull(kudu_tables_by_name, hms_table_name);
+      } else if (storage_handler == HmsClient::kKuduStorageHandler) {
+        // TODO(ghenke): Also lookup by name to support tables created when HMS sync is off.
+        // TODO(ghenke): Also lookup by name to support repairing id's out of sync.
+        const string& hms_table_id = hms_table.parameters[HmsClient::kKuduTableIdKey];
+        shared_ptr<KuduTable>* kudu_table = FindOrNull(kudu_tables_by_id,
+                                                       hms_table_id);
         if (kudu_table) {
-          hms_tables_by_id[(*kudu_table)->id()].emplace_back(std::move(hms_table));
+          managed_hms_tables_by_id[(*kudu_table)->id()].emplace_back(
+              std::move(hms_table));
         } else {
-          orphan_tables.emplace_back(std::move(hms_table));
+          orphan_hms_tables.emplace_back(std::move(hms_table));
         }
       }
     }
@@ -335,8 +350,11 @@ Status AnalyzeCatalogs(const string& master_addrs,
   vector<shared_ptr<KuduTable>> invalid_name_tables;
   for (auto& kudu_table_pair : kudu_tables_by_id) {
     shared_ptr<KuduTable> kudu_table = kudu_table_pair.second;
-    vector<hive::Table>* hms_tables = FindOrNull(hms_tables_by_id, kudu_table_pair.first);
-
+    // Check all of the managed HMS tables.
+    vector<hive::Table>* hms_tables = FindOrNull(managed_hms_tables_by_id,
+                                                 kudu_table_pair.first);
+    // If the there are no managed HMS table entries, this table is missing
+    // HMS tables and might have an invalid table name.
     if (!hms_tables) {
       const string& table_name = kudu_table->name();
       string normalized_table_name(table_name.data(), table_name.size());
@@ -346,6 +364,8 @@ Status AnalyzeCatalogs(const string& master_addrs,
       } else {
         missing_tables.emplace_back(std::move(kudu_table));
       }
+    // If there is a single managed HMS table, this table could be unsynced or
+    // using the legacy handler.
     } else if (hms_tables->size() == 1) {
       hive::Table& hms_table = (*hms_tables)[0];
       const string& storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
@@ -355,6 +375,7 @@ Status AnalyzeCatalogs(const string& master_addrs,
       } else if (storage_handler == HmsClient::kLegacyKuduStorageHandler) {
         legacy_tables.emplace_back(make_pair(std::move(kudu_table), std::move(hms_table)));
       }
+    // Otherwise, there are multiple managed HMS tables for a single Kudu table.
     } else {
       for (hive::Table& hms_table : *hms_tables) {
         duplicate_tables.emplace_back(make_pair(kudu_table, std::move(hms_table)));
@@ -362,12 +383,24 @@ Status AnalyzeCatalogs(const string& master_addrs,
     }
   }
 
-  report->orphan_hms_tables.swap(orphan_tables);
+  // Check all of the external HMS tables to see if they are using the legacy handler.
+  for (auto& hms_table_pair : external_hms_tables_by_id) {
+    shared_ptr<KuduTable>* kudu_table = FindOrNull(kudu_tables_by_id, hms_table_pair.first);
+    if (kudu_table) {
+      for (hive::Table &hms_table : hms_table_pair.second) {
+        const string &storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
+        if (storage_handler == HmsClient::kLegacyKuduStorageHandler) {
+          legacy_tables.emplace_back(make_pair(std::move(*kudu_table), std::move(hms_table)));
+        }
+      }
+    }
+  }
+  report->orphan_hms_tables.swap(orphan_hms_tables);
   report->missing_hms_tables.swap(missing_tables);
   report->invalid_name_tables.swap(invalid_name_tables);
+  report->inconsistent_tables.swap(stale_tables);
   report->legacy_hms_tables.swap(legacy_tables);
   report->duplicate_hms_tables.swap(duplicate_tables);
-  report->inconsistent_tables.swap(stale_tables);
   return Status::OK();
 }
 
@@ -407,9 +440,9 @@ Status CheckHmsMetadata(const RunnerContext& context) {
     cout << "Found Kudu table(s) with multiple corresponding Hive Metastore tables:" << endl;
     RETURN_NOT_OK(PrintTables(master_addrs, std::move(tables), cout));
     cout << endl
-         << "Suggestion: using Impala or the Hive Beeline shell, drop the duplicate Hive Metastore "
+         << "Suggestion: using Impala or the Hive Beeline shell, alter one of the duplicate"
          << endl
-         << "tables and consider recreating them as views referencing the base Kudu table."
+         << "Hive Metastore tables to be an external table referencing the base Kudu table."
          << endl
          << endl;
   }
@@ -574,7 +607,8 @@ Status FixHmsMetadata(const RunnerContext& context) {
               hms_table_name));
       }
 
-      if (kudu_table.name() != hms_table_name) {
+      if (hms_table.tableType == HmsClient::kManagedTable &&
+          kudu_table.name() != hms_table_name) {
         if (FLAGS_dryrun) {
           LOG(INFO) << "[dryrun] Renaming Kudu table " << TableIdent(kudu_table)
                     << " to " << hms_table_name;
@@ -704,6 +738,7 @@ Status Precheck(const RunnerContext& context) {
   return Status::IllegalState("found tables in Kudu with case-conflicting names");
 }
 
+// TODO(ghenke): Add dump tool that prints the Kudu HMS entries.
 unique_ptr<Mode> BuildHmsMode() {
   const string kHmsUrisDesc =
     "Address of the Hive Metastore instance(s). If not set, the configuration "


Mime
View raw message