You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2018/07/30 18:46:57 UTC

[1/4] kudu git commit: [util] update on ParseVersion()

Repository: kudu
Updated Branches:
  refs/heads/master 34b058b8b -> 8e40dfdb9


[util] update on ParseVersion()

This changelist updates implementation of the kudu::ParseVersion()
utility function to recognize and successfully parse version strings
like '1.8.0-cdh6.x-SNAPSHOT'.

Version::ToString() implementation changed to return
'canonical' representation of the parsed version string.  Added
corresponding unit tests.

This patch also fixes KUDU-2511.

This is a follow-up for c12e63528a2f2c265136e702a48cbc6c93b7f2fd.

Change-Id: I98b200d0529cb4471e4e855b88bfdb2ff2f346bf
Reviewed-on: http://gerrit.cloudera.org:8080/11060
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/7e733a73
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/7e733a73
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/7e733a73

Branch: refs/heads/master
Commit: 7e733a73652db68342859d03d02763ef5e3de79d
Parents: 34b058b
Author: Alexey Serbin <as...@cloudera.com>
Authored: Wed Jul 25 22:07:11 2018 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Mon Jul 30 01:55:52 2018 +0000

----------------------------------------------------------------------
 src/kudu/util/version_util-test.cc | 88 +++++++++++++++++++++++++++------
 src/kudu/util/version_util.cc      | 29 ++++++-----
 src/kudu/util/version_util.h       | 11 +++--
 3 files changed, 98 insertions(+), 30 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/7e733a73/src/kudu/util/version_util-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/version_util-test.cc b/src/kudu/util/version_util-test.cc
index 54e8e76..3cb39b9 100644
--- a/src/kudu/util/version_util-test.cc
+++ b/src/kudu/util/version_util-test.cc
@@ -14,53 +14,109 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
+
 #include "kudu/util/version_util.h"
 
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <gtest/gtest.h>
 
 #include "kudu/util/status.h"
 #include "kudu/util/test_macros.h"
-#include "kudu/util/test_util.h"
+#include "kudu/util/version_info.h"
 
 using std::string;
+using std::pair;
 using std::vector;
 
 namespace kudu {
 
-class VersionUtilTest : public KuduTest {};
-
-TEST_F(VersionUtilTest, TestVersion) {
-  const vector<Version> good_test_cases = {
-    { "0.0.0", 0, 0, 0, "" },
-    { "1.0.0", 1, 0, 0, "" },
-    { "1.1.0", 1, 1, 0, "" },
-    { "1.1.1", 1, 1, 1, "" },
-    { "1.10.100-1000", 1, 10, 100, "1000" },
-    { "1.2.3-SNAPSHOT", 1, 2, 3, "SNAPSHOT" },
+TEST(VersionUtilTest, TestVersion) {
+  const vector<pair<Version, string>> good_test_cases = {
+    { { "0.0.0", 0, 0, 0, "" }, "0.0.0" },
+    { { "1.0.0", 1, 0, 0, "" }, "1.0.0" },
+    { { "1.1.0", 1, 1, 0, "" }, "1.1.0" },
+    { { "1.1.1", 1, 1, 1, "" }, "1.1.1" },
+    { { "1.1.1-", 1, 1, 1, "" }, "1.1.1" },
+    { { "1.1.1-0-1-2", 1, 1, 1, "0-1-2" }, "1.1.1-0-1-2" },
+    { { "1.10.100-1000.0", 1, 10, 100, "1000.0" }, "1.10.100-1000.0" },
+    { { "1.2.3-SNAPSHOT", 1, 2, 3, "SNAPSHOT" }, "1.2.3-SNAPSHOT" },
+    { { "1.8.0-x-SNAPSHOT", 1, 8, 0, "x-SNAPSHOT" }, "1.8.0-x-SNAPSHOT" },
+    { { "0.1.2-a-b-c-d", 0, 1, 2, "a-b-c-d" }, "0.1.2-a-b-c-d" },
+    { { " 0 .1 .2 -x", 0, 1, 2, "x" }, "0.1.2-x" },
+    { { " 0 . 1 .  2 -x ", 0, 1, 2, "x" }, "0.1.2-x" },
+    { { "+1.+2.+3-6", 1, 2, 3, "6" }, "1.2.3-6" },
+    { { " +1 .  +2 .   +3 -100 .. ", 1, 2, 3, "100 .." }, "1.2.3-100 .." },
+    // no octals: leading zeros are just chopped off
+    { { "00.01.010", 0, 1, 10, "" }, "0.1.10" },
+    { { "  0.1.2----suffix  ", 0, 1, 2, "---suffix" }, "0.1.2----suffix" },
+    { { "0.1.2- - -x- -y- ", 0, 1, 2, " - -x- -y-" }, "0.1.2- - -x- -y-" },
   };
 
-  Version v;
   for (const auto& test_case : good_test_cases) {
-    ASSERT_OK(ParseVersion(test_case.raw_version, &v));
-    EXPECT_EQ(test_case, v);
+    const auto& version = test_case.first;
+    const auto& canonical_str = test_case.second;
+    Version v;
+    ASSERT_OK(ParseVersion(version.raw_version, &v));
+    EXPECT_EQ(version, v);
+    EXPECT_EQ(canonical_str, v.ToString());
   }
 
   const vector<string> bad_test_cases = {
     "",
+    " ",
+    "-",
+    " -",
+    "--",
+    " - - ",
+    "..",
+    "0.1",
+    "0.1.",
+    "0.1.+",
+    "+ 0.+ 1.+ 2",
+    "0.1.+-woops",
+    "0.1.2+",
+    "1..1",
+    ".0.1",
+    " . . ",
+    "-0.1.2-013",
+    "0.-1.2-013",
+    "0.1.-2-013",
+    "1000,000.2999,999.999",
+    "1e10.0.1",
+    "1e+10.0.1",
+    "1e-1.0.1",
+    "1E+1.2.3",
+    "1E-5.0.1",
     "foo",
     "foo.1.0",
+    "a.b.c",
+    "0.x1.x2",
+    "0x0.0x1.0x2",
     "1.bar.0",
     "1.0.foo",
     "1.0foo.bar",
     "foo5-1.4.3",
   };
 
-  for (const auto& test_case : bad_test_cases) {
-    ASSERT_TRUE(ParseVersion(test_case, &v).IsInvalidArgument());
+  for (const auto& input_str : bad_test_cases) {
+    Version v;
+    const auto s = ParseVersion(input_str, &v);
+    ASSERT_TRUE(s.IsInvalidArgument())
+        << s.ToString() << ": " << input_str << " ---> " << v.ToString();
   }
 }
 
+// Sanity check: parse current Kudu version string and make sure the 'canonical'
+// representation of the parsed version matches the 'raw' input as is.
+TEST(VersionUtilTest, ParseCurrentKuduVersionString) {
+  const auto ver_string = VersionInfo::GetShortVersionInfo();
+  Version v;
+  const auto s = ParseVersion(ver_string, &v);
+  ASSERT_TRUE(s.ok()) << s.ToString();
+  EXPECT_EQ(ver_string, v.ToString());
+}
+
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/7e733a73/src/kudu/util/version_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/version_util.cc b/src/kudu/util/version_util.cc
index bd298f8..6715894 100644
--- a/src/kudu/util/version_util.cc
+++ b/src/kudu/util/version_util.cc
@@ -17,15 +17,17 @@
 
 #include "kudu/util/version_util.h"
 
-#include <iostream>
+#include <iterator>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include <glog/logging.h>
 
+#include "kudu/gutil/strings/join.h"
 #include "kudu/gutil/strings/numbers.h"
 #include "kudu/gutil/strings/split.h"
+#include "kudu/gutil/strings/strip.h"
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/util/status.h"
 
@@ -45,7 +47,9 @@ bool Version::operator==(const Version& other) const {
 }
 
 string Version::ToString() const {
-  return raw_version;
+  return extra.empty()
+      ? Substitute("$0.$1.$2", major, minor, maintenance)
+      : Substitute("$0.$1.$2-$3", major, minor, maintenance, extra);
 }
 
 ostream& operator<<(ostream& os, const Version& v) {
@@ -54,29 +58,32 @@ ostream& operator<<(ostream& os, const Version& v) {
 
 Status ParseVersion(const string& version_str,
                     Version* v) {
-  CHECK(v);
+  static const char* const kDelimiter = "-";
+
+  DCHECK(v);
   const Status invalid_ver_err =
       Status::InvalidArgument("invalid version string", version_str);
-  Version temp_v;
-  const vector<string> main_and_extra = Split(version_str, "-");
-  if (main_and_extra.empty() || main_and_extra.size() > 2) {
+  auto v_str = version_str;
+  StripWhiteSpace(&v_str);
+  const vector<string> main_and_extra = Split(v_str, kDelimiter);
+  if (main_and_extra.empty()) {
     return invalid_ver_err;
   }
-  if (main_and_extra.size() == 2) {
-    temp_v.extra = main_and_extra[1];
-  }
-  const auto& main_ver_str = main_and_extra[0];
-  const vector<string> maj_min_maint = Split(main_ver_str, ".");
+  const vector<string> maj_min_maint = Split(main_and_extra.front(), ".");
   if (maj_min_maint.size() != 3) {
     return invalid_ver_err;
   }
+  Version temp_v;
   if (!SimpleAtoi(maj_min_maint[0], &temp_v.major) ||
       !SimpleAtoi(maj_min_maint[1], &temp_v.minor) ||
       !SimpleAtoi(maj_min_maint[2], &temp_v.maintenance)) {
     return invalid_ver_err;
   }
+  temp_v.extra = JoinStringsIterator(std::next(main_and_extra.begin()),
+                                     main_and_extra.end(), kDelimiter);
   temp_v.raw_version = version_str;
   *v = std::move(temp_v);
+
   return Status::OK();
 }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/7e733a73/src/kudu/util/version_util.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/version_util.h b/src/kudu/util/version_util.h
index 5cde6cc..446934c 100644
--- a/src/kudu/util/version_util.h
+++ b/src/kudu/util/version_util.h
@@ -16,9 +16,10 @@
 // under the License.
 #pragma once
 
-#include <iostream>
+#include <iosfwd>
 #include <string>
 
+#include "kudu/gutil/port.h"
 #include "kudu/util/status.h"
 
 namespace kudu {
@@ -27,7 +28,7 @@ namespace kudu {
 //
 //  <major>.<minor>.<maintenance>[-<extra>]
 //
-// e.g. 1.6.0 or 1.7.1-SNAPSHOT.
+// e.g. 1.6.0, 1.7.1-SNAPSHOT, 1.8.0-RC1-SNAPSHOT, etc.
 //
 // This struct can be used with versions reported by ksck to determine if and
 // how certain tools should function depending on what versions are running in
@@ -35,6 +36,10 @@ namespace kudu {
 struct Version {
   bool operator==(const Version& other) const;
 
+  // Return 'canonical' version string, i.e. the concatenation of the version
+  // components transformed back into the string representation. The parser
+  // implementation has its quirks, so the canonical version string does not
+  // always match the raw input string.
   std::string ToString() const;
 
   // The original version string.
@@ -53,6 +58,6 @@ std::ostream& operator<<(std::ostream& os, const Version& v);
 
 // Parse 'version_str' into 'v'. 'v' must not be null.
 Status ParseVersion(const std::string& version_str,
-                    Version* v);
+                    Version* v) WARN_UNUSED_RESULT;
 
 } // namespace kudu


[2/4] kudu git commit: [kudu-admin-test] improvements on error reporting

Posted by da...@apache.org.
[kudu-admin-test] improvements on error reporting

This changelist introduces improvements on logging in case
if an assertion triggers in the rebalancer-related scenarios.

I found it's hard to troubleshoot the flakiness reported lately
in the RebalancerAndSingleReplicaTablets.SingleReplicasStayOrMove
scenario.  With hindsight, it turned out that improved logging
on assertions would help a lot.

Change-Id: Ibfa74d5a78e89bf7ff1a0e914384999466460145
Reviewed-on: http://gerrit.cloudera.org:8080/11046
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/660ee80b
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/660ee80b
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/660ee80b

Branch: refs/heads/master
Commit: 660ee80bb4c0f81cb6f0a08365a7e2abd0a3e409
Parents: 7e733a7
Author: Alexey Serbin <as...@cloudera.com>
Authored: Tue Jul 24 20:20:00 2018 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Mon Jul 30 15:06:44 2018 +0000

----------------------------------------------------------------------
 src/kudu/tools/kudu-admin-test.cc | 80 ++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 33 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/660ee80b/src/kudu/tools/kudu-admin-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc
index 21bae3b..0408e35 100644
--- a/src/kudu/tools/kudu-admin-test.cc
+++ b/src/kudu/tools/kudu-admin-test.cc
@@ -48,7 +48,6 @@
 #include "kudu/gutil/map-util.h"
 #include "kudu/gutil/strings/split.h"
 #include "kudu/gutil/strings/substitute.h"
-#include "kudu/gutil/strings/util.h"
 #include "kudu/integration-tests/cluster_itest_util.h"
 #include "kudu/integration-tests/cluster_verifier.h"
 #include "kudu/integration-tests/test_workload.h"
@@ -105,6 +104,7 @@ using std::atomic;
 using std::back_inserter;
 using std::copy;
 using std::deque;
+using std::endl;
 using std::ostringstream;
 using std::string;
 using std::thread;
@@ -1331,6 +1331,14 @@ TEST_F(AdminCliTest, TestListTablesDetail) {
   }
 }
 
+static string ToolRunInfo(const Status& s, const string& out, const string& err) {
+  ostringstream str;
+  str << s.ToString() << endl;
+  str << "stdout: " << out << endl;
+  str << "stderr: " << err << endl;
+  return str.str();
+}
+
 // Make sure the rebalancer doesn't start if a tablet server is down.
 class RebalanceStartCriteriaTest :
     public AdminCliTest,
@@ -1359,13 +1367,14 @@ TEST_P(RebalanceStartCriteriaTest, TabletServerIsDown) {
     ts->Shutdown();
   }
 
+  string out;
   string err;
   Status s = RunKuduTool({
     "cluster",
     "rebalance",
     cluster_->master()->bound_rpc_addr().ToString()
-  }, nullptr, &err);
-  ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+  }, &out, &err);
+  ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, out, err);
   const auto err_msg_pattern = Substitute(
       "Illegal state: tablet server .* \\($0\\): "
       "unacceptable health status UNAVAILABLE",
@@ -1501,7 +1510,7 @@ TEST_P(RebalanceParamTest, Rebalance) {
     string out;
     string err;
     const Status s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString() << ":" << err;
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced")
         << "stderr: " << err;
   }
@@ -1512,7 +1521,7 @@ TEST_P(RebalanceParamTest, Rebalance) {
     string out;
     string err;
     const Status s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString() << ":" << err;
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out,
         "rebalancing is complete: cluster is balanced (moved 0 replicas)")
         << "stderr: " << err;
@@ -1532,7 +1541,7 @@ TEST_P(RebalanceParamTest, Rebalance) {
     string out;
     string err;
     const Status s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString() << ":" << err;
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced")
         << "stderr: " << err;
     // The cluster was un-balanced, so many replicas should have been moved.
@@ -1570,6 +1579,8 @@ class RebalancingTest :
   }
 
  protected:
+  static const char* const kExitOnSignalStr;
+
   void Prepare(const vector<string>& extra_tserver_flags = {},
                const vector<string>& extra_master_flags = {}) {
     copy(extra_tserver_flags.begin(), extra_tserver_flags.end(),
@@ -1616,6 +1627,7 @@ class RebalancingTest :
   vector<string> tserver_flags_;
   vector<string> master_flags_;
 };
+const char* const RebalancingTest::kExitOnSignalStr = "kudu: process exited on signal";
 
 // Make sure the rebalancer is able to do its job if running concurrently
 // with DDL activity on the cluster.
@@ -1781,7 +1793,7 @@ TEST_P(DDLDuringRebalancingTest, TablesCreatedAndDeletedDuringRebalancing) {
     string out;
     string err;
     const auto s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString() << ": " << err;
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced")
         << "stderr: " << err;
   }
@@ -1792,7 +1804,7 @@ TEST_P(DDLDuringRebalancingTest, TablesCreatedAndDeletedDuringRebalancing) {
     string out;
     string err;
     const auto s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString() << ": " << err;
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out,
         "rebalancing is complete: cluster is balanced (moved 0 replicas)")
         << "stderr: " << err;
@@ -1829,21 +1841,18 @@ TEST_P(ConcurrentRebalancersTest, TwoConcurrentRebalancers) {
   };
 
   const auto runner_func = [&]() {
+    string out;
     string err;
-    const auto s = RunKuduTool(tool_args, nullptr, &err);
-
-    ostringstream os;
-    os << "rebalancer status: " << s.ToString();
-    // One might expect a bad status returned: e.g., due to some race so
-    // the rebalancer didn't able to make progress for more than
-    // --max_staleness_interval_sec, etc.
+    const auto s = RunKuduTool(tool_args, &out, &err);
     if (!s.ok()) {
-      os << " : " << err;
+      // One might expect a bad status returned: e.g., due to some race so
+      // the rebalancer didn't able to make progress for more than
+      // --max_staleness_interval_sec, etc.
+      LOG(INFO) << "rebalancer run info: " << ToolRunInfo(s, out, err);
     }
-    LOG(INFO) << os.str();
 
     // Should not exit on a signal: not expecting SIGSEGV, SIGABRT, etc.
-    return !MatchPattern(err, "*kudu: process exited on signal*");
+    return s.ToString().find(kExitOnSignalStr) == string::npos;
   };
 
   CountDownLatch start_synchronizer(1);
@@ -1868,7 +1877,7 @@ TEST_P(ConcurrentRebalancersTest, TwoConcurrentRebalancers) {
     string out;
     string err;
     const auto s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString() << ": " << err;
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced")
         << "stderr: " << err;
   }
@@ -1881,7 +1890,7 @@ TEST_P(ConcurrentRebalancersTest, TwoConcurrentRebalancers) {
     string out;
     string err;
     const auto s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString() << ": " << err;
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out,
         "rebalancing is complete: cluster is balanced (moved 0 replicas)")
         << "stderr: " << err;
@@ -1921,13 +1930,14 @@ TEST_P(TserverGoesDownDuringRebalancingTest, TserverDown) {
   // Pre-condition: 'kudu cluster ksck' should be happy with the cluster state
   // shortly after initial setup.
   ASSERT_EVENTUALLY([&]() {
+    string out;
     string err;
     const auto s = RunKuduTool({
       "cluster",
       "ksck",
       cluster_->master()->bound_rpc_addr().ToString(),
-    }, nullptr, &err);
-    ASSERT_TRUE(s.ok()) << "stderr: " << err;
+    }, &out, &err);
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
   });
 
   Random r(SeedRandom());
@@ -1960,13 +1970,13 @@ TEST_P(TserverGoesDownDuringRebalancingTest, TserverDown) {
       // server goes down.
       "--max_moves_per_server=1",
     }, &out, &err);
-    ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+    ASSERT_TRUE(s.IsRuntimeError()) << ToolRunInfo(s, out, err);
+
+    // The rebalancer tool should not crash.
+    ASSERT_STR_NOT_CONTAINS(s.ToString(), kExitOnSignalStr);
     ASSERT_STR_MATCHES(
         err, "Illegal state: tablet server .* \\(.*\\): "
              "unacceptable health status UNAVAILABLE");
-
-    // The rebalancer tool should not crash.
-    ASSERT_STR_NOT_CONTAINS(err, "kudu: process exited on signal");
   }
 
   run = false;
@@ -2011,9 +2021,10 @@ TEST_P(TserverAddedDuringRebalancingTest, TserverStarts) {
   atomic<bool> run(true);
   thread runner([&]() {
     while (run) {
+      string out;
       string err;
-      const auto s = RunKuduTool(tool_args, nullptr, &err);
-      CHECK(s.ok()) << s.ToString() << "stderr: " << err;
+      const auto s = RunKuduTool(tool_args, &out, &err);
+      CHECK(s.ok()) << ToolRunInfo(s, out, err);
     }
   });
   auto runner_cleanup = MakeScopedCleanup([&]() {
@@ -2037,7 +2048,7 @@ TEST_P(TserverAddedDuringRebalancingTest, TserverStarts) {
     string out;
     string err;
     const auto s = RunKuduTool(tool_args, &out, &err);
-    ASSERT_TRUE(s.ok()) << s.ToString();
+    ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
     ASSERT_STR_CONTAINS(out,
         "rebalancing is complete: cluster is balanced (moved 0 replicas)")
         << "stderr: " << err;
@@ -2125,17 +2136,20 @@ TEST_P(RebalancerAndSingleReplicaTablets, SingleReplicasStayOrMove) {
     cluster_->master()->bound_rpc_addr().ToString(),
     Substitute("--move_single_replicas=$0", move_single_replica),
   }, &out, &err);
-  ASSERT_TRUE(s.ok()) << s.ToString() << ":" << err;
-  ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced");
+  ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err);
+  ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced")
+      << "stderr: " << err;
   if (move_single_replica == "enabled" ||
       (move_single_replica == "auto" && is_343_scheme)) {
     // Should move appropriate replicas of single-replica tablets.
     ASSERT_STR_NOT_CONTAINS(out,
-        "rebalancing is complete: cluster is balanced (moved 0 replicas)");
+        "rebalancing is complete: cluster is balanced (moved 0 replicas)")
+        << "stderr: " << err;
     ASSERT_STR_NOT_CONTAINS(err, "has single replica, skipping");
   } else {
     ASSERT_STR_CONTAINS(out,
-        "rebalancing is complete: cluster is balanced (moved 0 replicas)");
+        "rebalancing is complete: cluster is balanced (moved 0 replicas)")
+        << "stderr: " << err;
     ASSERT_STR_MATCHES(err, "tablet .* of table '.*' (.*) has single replica, skipping");
   }
 


[4/4] kudu git commit: hms-tool: refactor check tool and combine upgrade and fix

Posted by da...@apache.org.
hms-tool: refactor check tool and combine upgrade and fix

This commit combines the 'hms upgrade' and 'hms fix' tools. The tools
previously had overlapping responsibilities for checking some types of
inconsistencies. The 'fix' tool now has flags which can control whether
it attempts certain types of fixes:

  --drop_orphan_hms_tables=false
  --create_missing_hms_tables=true
  --fix_inconsistent_tables=true
  --upgrade_hms_tables=true

`drop_orphan_hms_tables` defaults to false since it deletes Hive tables,
and the tool can not rule-out that they are being used. Additionally, a
--dryrun flag is provided in order to print steps that would be taken
without actually making modifications.

The checking logic has been expanded to account for more potential
inconsistencies, and where possible the fix tool now can automatically
repair these inconsistencies.

Finally, the input prompt and default database functionality has been
removed in order to simplify the tool. Instead, the check tool will
issue hints including instructions for how to rename tables without a
database or with a Hive-incompatible name using the
'kudu table rename_table' tool. We can always add this functionality
back in the future if we determine it helps out users.

Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f
Reviewed-on: http://gerrit.cloudera.org:8080/11018
Tested-by: Dan Burkert <da...@apache.org>
Reviewed-by: Hao Hao <ha...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/8e40dfdb
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/8e40dfdb
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/8e40dfdb

Branch: refs/heads/master
Commit: 8e40dfdb97ee54906dfa519e28fbcffc290095ac
Parents: 660ee80
Author: Dan Burkert <da...@apache.org>
Authored: Fri Jul 6 17:04:10 2018 -0700
Committer: Dan Burkert <da...@apache.org>
Committed: Mon Jul 30 18:46:28 2018 +0000

----------------------------------------------------------------------
 src/kudu/gutil/strings/escaping.h |  10 +-
 src/kudu/hms/hms_catalog.cc       |  14 +-
 src/kudu/hms/hms_catalog.h        |  17 +-
 src/kudu/tools/CMakeLists.txt     |   7 +-
 src/kudu/tools/kudu-tool-test.cc  | 777 ++++++++++++++---------------
 src/kudu/tools/tool_action_hms.cc | 888 ++++++++++++++++++---------------
 6 files changed, 891 insertions(+), 822 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/8e40dfdb/src/kudu/gutil/strings/escaping.h
----------------------------------------------------------------------
diff --git a/src/kudu/gutil/strings/escaping.h b/src/kudu/gutil/strings/escaping.h
index c39bf8a..1f40f68 100644
--- a/src/kudu/gutil/strings/escaping.h
+++ b/src/kudu/gutil/strings/escaping.h
@@ -142,7 +142,15 @@ bool CUnescape(const StringPiece& source, std::string* dest, std::string* error)
 
 // A version with no error reporting.
 inline bool CUnescape(const StringPiece& source, std::string* dest) {
-  return CUnescape(source, dest, NULL);
+  return CUnescape(source, dest, nullptr);
+}
+
+// A version which CHECK fails if the string can not be unescaped.
+inline std::string CUnescapeOrDie(const StringPiece& source) {
+  std::string dest;
+  std::string err;
+  CHECK(CUnescape(source, &dest, &err)) << err;
+  return dest;
 }
 
 // ----------------------------------------------------------------------

http://git-wip-us.apache.org/repos/asf/kudu/blob/8e40dfdb/src/kudu/hms/hms_catalog.cc
----------------------------------------------------------------------
diff --git a/src/kudu/hms/hms_catalog.cc b/src/kudu/hms/hms_catalog.cc
index 11faec0..deabb13 100644
--- a/src/kudu/hms/hms_catalog.cc
+++ b/src/kudu/hms/hms_catalog.cc
@@ -151,13 +151,19 @@ Status HmsCatalog::CreateTable(const string& id,
 }
 
 Status HmsCatalog::DropTable(const string& id, const string& name) {
-  Slice hms_database;
-  Slice hms_table;
-  RETURN_NOT_OK(ParseTableName(name, &hms_database, &hms_table));
-
   hive::EnvironmentContext env_ctx = EnvironmentContext();
   env_ctx.properties.insert(make_pair(HmsClient::kKuduTableIdKey, id));
+  return DropTable(name, env_ctx);
+}
 
+Status HmsCatalog::DropLegacyTable(const string& name) {
+  return DropTable(name, EnvironmentContext());
+}
+
+Status HmsCatalog::DropTable(const string& name, const hive::EnvironmentContext& env_ctx) {
+  Slice hms_database;
+  Slice hms_table;
+  RETURN_NOT_OK(ParseTableName(name, &hms_database, &hms_table));
   return Execute([&] (HmsClient* client) {
     return client->DropTable(hms_database.ToString(), hms_table.ToString(), env_ctx);
   });

http://git-wip-us.apache.org/repos/asf/kudu/blob/8e40dfdb/src/kudu/hms/hms_catalog.h
----------------------------------------------------------------------
diff --git a/src/kudu/hms/hms_catalog.h b/src/kudu/hms/hms_catalog.h
index f75ee3e..ebb1f41 100644
--- a/src/kudu/hms/hms_catalog.h
+++ b/src/kudu/hms/hms_catalog.h
@@ -75,8 +75,17 @@ class HmsCatalog {
   // This method will fail if the HMS is unreachable, if the table does not
   // exist in the HMS, or if the table entry in the HMS doesn't match the
   // specified Kudu table ID.
-  Status DropTable(const std::string& id,
-                   const std::string& name) WARN_UNUSED_RESULT;
+  Status DropTable(const std::string& id, const std::string& name) WARN_UNUSED_RESULT;
+
+  // Drops a legacy table from the HMS.
+  //
+  // This method will fail if the HMS is unreachable, or if the table does not
+  // exist in the HMS.
+  //
+  // Note: it's possible to drop a non-legacy table using this method, but that
+  // should be avoided, since it will skip the table ID checks in the Kudu HMS
+  // plugin.
+  Status DropLegacyTable(const std::string& name) WARN_UNUSED_RESULT;
 
   // Alters a table entry in the HMS.
   //
@@ -165,6 +174,10 @@ class HmsCatalog {
   // are unavailable.
   Status Reconnect();
 
+  // Drops a table entry from the HMS, supplying the provided environment context.
+  Status DropTable(const std::string& name,
+                   const hive::EnvironmentContext& env_ctx) WARN_UNUSED_RESULT;
+
   // Returns true if the RPC status is 'fatal', e.g. the Thrift connection on
   // which it occurred should be shut down.
   static bool IsFatalError(const Status& status);

http://git-wip-us.apache.org/repos/asf/kudu/blob/8e40dfdb/src/kudu/tools/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/tools/CMakeLists.txt b/src/kudu/tools/CMakeLists.txt
index cfc3ce0..3f6020f 100644
--- a/src/kudu/tools/CMakeLists.txt
+++ b/src/kudu/tools/CMakeLists.txt
@@ -154,11 +154,12 @@ target_link_libraries(kudu_tools_test_util
 #######################################
 
 set(KUDU_TEST_LINK_LIBS
+  itest_util
   ksck
-  kudu_tools_util
-  kudu_tools_test_util
+  kudu_hms
   kudu_tools_rebalance
-  itest_util
+  kudu_tools_test_util
+  kudu_tools_util
   mini_cluster
   ${KUDU_MIN_TEST_LIBS})
 ADD_KUDU_TEST(diagnostics_log_parser-test)

http://git-wip-us.apache.org/repos/asf/kudu/blob/8e40dfdb/src/kudu/tools/kudu-tool-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index f9ca66b..bb13efa 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -29,6 +29,7 @@
 #include <tuple>  // IWYU pragma: keep
 #include <type_traits>
 #include <unordered_map>
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -75,8 +76,10 @@
 #include "kudu/gutil/strings/strcat.h"
 #include "kudu/gutil/strings/strip.h"
 #include "kudu/gutil/strings/substitute.h"
+#include "kudu/gutil/strings/escaping.h"
 #include "kudu/gutil/strings/util.h"
 #include "kudu/hms/hive_metastore_types.h"
+#include "kudu/hms/hms_catalog.h"
 #include "kudu/hms/hms_client.h"
 #include "kudu/hms/mini_hms.h"
 #include "kudu/integration-tests/cluster_itest_util.h"
@@ -124,6 +127,7 @@
 #include "kudu/util/url-coding.h"
 
 DECLARE_string(block_manager);
+DECLARE_string(hive_metastore_uris);
 
 METRIC_DECLARE_counter(bloom_lookups);
 METRIC_DECLARE_entity(tablet);
@@ -149,6 +153,7 @@ using kudu::consensus::ReplicateRefPtr;
 using kudu::fs::BlockDeletionTransaction;
 using kudu::fs::FsReport;
 using kudu::fs::WritableBlock;
+using kudu::hms::HmsCatalog;
 using kudu::hms::HmsClient;
 using kudu::hms::HmsClientOptions;
 using kudu::itest::MiniClusterFsInspector;
@@ -176,6 +181,7 @@ using std::pair;
 using std::string;
 using std::unique_ptr;
 using std::unordered_map;
+using std::unordered_set;
 using std::vector;
 using strings::Substitute;
 
@@ -2078,26 +2084,20 @@ TEST_F(ToolTest, TestRenameColumn) {
 }
 
 Status CreateLegacyHmsTable(HmsClient* client,
-                            const string& database_name,
-                            const string& table_name,
+                            const string& hms_database_name,
+                            const string& hms_table_name,
+                            const string& kudu_table_name,
+                            const string& kudu_master_addrs,
                             const string& table_type) {
   hive::Table table;
-  string kudu_table_name(table_name);
-  table.dbName = database_name;
-  table.tableName = table_name;
+  table.dbName = hms_database_name;
+  table.tableName = hms_table_name;
   table.tableType = table_type;
-  if (table_type == HmsClient::kManagedTable) {
-    kudu_table_name = Substitute("$0$1.$2", HmsClient::kLegacyTablePrefix,
-                                 database_name, table_name);
-  }
 
   table.__set_parameters({
-      make_pair(HmsClient::kStorageHandlerKey,
-                HmsClient::kLegacyKuduStorageHandler),
-      make_pair(HmsClient::kLegacyKuduTableNameKey,
-                kudu_table_name),
-      make_pair(HmsClient::kKuduMasterAddrsKey,
-                "Master_Addrs"),
+      make_pair(HmsClient::kStorageHandlerKey, HmsClient::kLegacyKuduStorageHandler),
+      make_pair(HmsClient::kLegacyKuduTableNameKey, kudu_table_name),
+      make_pair(HmsClient::kKuduMasterAddrsKey, kudu_master_addrs),
   });
 
   // TODO(Hao): Remove this once HIVE-19253 is fixed.
@@ -2120,12 +2120,9 @@ Status CreateHmsTable(HmsClient* client,
   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),
+      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.
@@ -2169,140 +2166,11 @@ void ValidateHmsEntries(HmsClient* hms_client,
   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_TRUE(hms_table.parameters[HmsClient::kStorageHandlerKey] ==
-                  HmsClient::kKuduStorageHandler &&
-              hms_table.parameters[HmsClient::kKuduTableIdKey] ==
-                  kudu_table->id() &&
-              hms_table.parameters[HmsClient::kKuduMasterAddrsKey] == master_addr &&
-              !ContainsKey(hms_table.parameters, HmsClient::kLegacyKuduTableNameKey));
-}
-
-TEST_F(ToolTest, TestHmsUpgrade) {
-  ExternalMiniClusterOptions opts;
-  opts.hms_mode = HmsMode::ENABLE_HIVE_METASTORE;
-  NO_FATALS(StartExternalMiniCluster(std::move(opts)));
-
-  string master_addr = cluster_->master()->bound_rpc_addr().ToString();
-  HmsClient hms_client(cluster_->hms()->address(), HmsClientOptions());
-  ASSERT_OK(hms_client.Start());
-  ASSERT_TRUE(hms_client.IsConnected());
-
-  const string kDatabaseName = "my_db";
-  const string kDefaultDatabaseName = "default";
-  const string kManagedTableName = "managed_table";
-  const string kExternalTableName = "external_table";
-  const string kKuduTableName = "kudu_table";
-  shared_ptr<KuduClient> kudu_client;
-  ASSERT_OK(KuduClientBuilder()
-      .add_master_server_addr(master_addr)
-      .Build(&kudu_client));
-
-  // 1. Create a managed impala table in HMS and the corresponding table in Kudu.
-  {
-    string legacy_managed_table_name = Substitute("$0$1.$2", HmsClient::kLegacyTablePrefix,
-                                                  kDatabaseName, kManagedTableName);
-    ASSERT_OK(CreateKuduTable(kudu_client, legacy_managed_table_name));
-    shared_ptr<KuduTable> table;
-    ASSERT_OK(kudu_client->OpenTable(legacy_managed_table_name, &table));
-    hive::Database db;
-    db.name = kDatabaseName;
-    ASSERT_OK(hms_client.CreateDatabase(db));
-    ASSERT_OK(CreateLegacyHmsTable(&hms_client, kDatabaseName, kManagedTableName,
-                                   HmsClient::kManagedTable));
-    hive::Table hms_table;
-    ASSERT_OK(hms_client.GetTable(kDatabaseName, kManagedTableName, &hms_table));
-    ASSERT_EQ(HmsClient::kManagedTable, hms_table.tableType);
-  }
-
-  // 2. Create an external impala table in HMS and the corresponding table in Kudu.
-  {
-    ASSERT_OK(CreateKuduTable(kudu_client, kExternalTableName));
-    shared_ptr<KuduTable> table;
-    ASSERT_OK(kudu_client->OpenTable(kExternalTableName, &table));
-    ASSERT_OK(CreateLegacyHmsTable(&hms_client, kDatabaseName, kExternalTableName,
-                                   HmsClient::kExternalTable));
-    hive::Table hms_table;
-    ASSERT_OK(hms_client.GetTable(kDatabaseName, kExternalTableName, &hms_table));
-    ASSERT_EQ(HmsClient::kExternalTable, hms_table.tableType);
-  }
-
-  // 3. Create several non-impala Kudu tables. One with hive compatible name, and the
-  //    other ones with hive incompatible names.
-  {
-    ASSERT_OK(CreateKuduTable(kudu_client, kKuduTableName));
-    shared_ptr<KuduTable> table;
-    ASSERT_OK(kudu_client->OpenTable(kKuduTableName, &table));
-
-    ASSERT_OK(CreateKuduTable(kudu_client, "invalid#table"));
-    ASSERT_OK(kudu_client->OpenTable("invalid#table", &table));
-
-    ASSERT_OK(CreateKuduTable(kudu_client, "invalid.@"));
-    ASSERT_OK(kudu_client->OpenTable("invalid.@", &table));
-
-    ASSERT_OK(CreateKuduTable(kudu_client, "@.invalid"));
-    ASSERT_OK(kudu_client->OpenTable("@.invalid", &table));
-
-    ASSERT_OK(CreateKuduTable(kudu_client, "invalid"));
-    ASSERT_OK(kudu_client->OpenTable("invalid", &table));
-
-    ASSERT_OK(CreateKuduTable(kudu_client, "in.val.id"));
-    ASSERT_OK(kudu_client->OpenTable("in.val.id", &table));
-  }
-
-  {
-    vector<string> table_names;
-    ASSERT_OK(hms_client.GetTableNames(kDatabaseName, &table_names));
-    ASSERT_EQ(2, table_names.size());
-  }
-
-  // Restart external mini cluster to enable Hive Metastore integration.
-  cluster_->EnableMetastoreIntegration();
-  cluster_->ShutdownNodes(cluster::ClusterNodes::ALL);
-  ASSERT_OK(cluster_->Restart());
-
-  // Upgrade the historical metadata in both Hive Metastore and Kudu.
-  string out;
-  NO_FATALS(RunActionStdinStdoutString(
-      Substitute("hms upgrade $0 $1 --unlock_experimental_flags=true "
-                 "--hive_metastore_uris=$2", master_addr,
-                 kDefaultDatabaseName, cluster_->hms()->uris()),
-      "valid_1\nvalid_2\nvalid_3\nvalid_4\nvalid_5\n", &out));
-
-  // Validate the Kudu table names and metadata format of hms entries.
-  {
-    vector<string> table_names;
-    ASSERT_OK(kudu_client->ListTables(&table_names));
-    ASSERT_EQ(8, table_names.size());
-    for (const auto& n : table_names) {
-      ASSERT_TRUE(IsValidTableName(n));
-    }
-
-    NO_FATALS(ValidateHmsEntries(&hms_client, kudu_client, kDatabaseName,
-                                 kManagedTableName, master_addr));
-    NO_FATALS(ValidateHmsEntries(&hms_client, kudu_client, kDatabaseName,
-                                 kExternalTableName, master_addr));
-    NO_FATALS(ValidateHmsEntries(&hms_client, kudu_client, kDefaultDatabaseName,
-                                 kKuduTableName, master_addr));
-    table_names.clear();
-    vector<string> db_names;
-    ASSERT_OK(hms_client.GetAllDatabases(&db_names));
-    ASSERT_EQ(2, db_names.size());
-    ASSERT_OK(hms_client.GetTableNames(kDatabaseName, &table_names));
-    ASSERT_EQ(2, table_names.size());
-    table_names.clear();
-    ASSERT_OK(hms_client.GetTableNames(kDefaultDatabaseName, &table_names));
-    ASSERT_EQ(6, table_names.size());
-
-    string out;
-    NO_FATALS(RunActionStdoutString(Substitute("hms check $0 --unlock_experimental_flags=true "
-                                               "--hive_metastore_uris=$1",
-                                               master_addr, cluster_->hms()->uris()), &out));
-    ASSERT_STR_CONTAINS(out, "OK");
-  }
-
-  ASSERT_OK(hms_client.Stop());
+  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));
 }
 
 TEST_F(ToolTest, TestHmsDowngrade) {
@@ -2315,54 +2183,38 @@ TEST_F(ToolTest, TestHmsDowngrade) {
   ASSERT_OK(hms_client.Start());
   ASSERT_TRUE(hms_client.IsConnected());
   shared_ptr<KuduClient> kudu_client;
-  ASSERT_OK(KuduClientBuilder()
-      .add_master_server_addr(master_addr)
-      .Build(&kudu_client));
+  ASSERT_OK(cluster_->CreateClient(nullptr, &kudu_client));
 
-  {
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.a"));
-    shared_ptr<KuduTable> kudu_table;
-    ASSERT_OK(kudu_client->OpenTable("default.a", &kudu_table));
-    hive::Table hms_table;
-    ASSERT_OK(hms_client.GetTable("default", "a", &hms_table));
+  string base_flags = Substitute("$0 --unlock_experimental_flags --hive_metastore_uris=$1",
+                                 master_addr, cluster_->hms()->uris());
 
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.b"));
-    ASSERT_OK(kudu_client->OpenTable("default.b", &kudu_table));
-    ASSERT_OK(hms_client.GetTable("default", "b", &hms_table));
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.a"));
+  NO_FATALS(ValidateHmsEntries(&hms_client, kudu_client, "default", "a", master_addr));
 
-    // Downgrade to legacy table in both Hive Metastore and Kudu.
-    NO_FATALS(RunActionStdoutNone(Substitute("hms downgrade $0 "
-                                             "--unlock_experimental_flags=true "
-                                             "--hive_metastore_uris=$1", master_addr,
-                                             cluster_->hms()->uris())));
-  }
+  // Downgrade to legacy table in both Hive Metastore and Kudu.
+  NO_FATALS(RunActionStdoutNone(Substitute("hms downgrade $0", base_flags)));
 
-  {
-    // Upgrade the metadata in both the Hive Metastore and Kudu.
-    string out;
-    NO_FATALS(RunActionStdinStdoutString(
-        Substitute("hms upgrade $0 $1 --unlock_experimental_flags=true "
-                   "--hive_metastore_uris=$2", master_addr, "default",
-                   cluster_->hms()->uris()),
-        "", &out));
+  // The check tool should report the legacy table.
+  string out;
+  string err;
+  Status s = RunActionStdoutStderrString(Substitute("hms check $0", base_flags), &out, &err);
+  ASSERT_FALSE(s.ok());
+  ASSERT_STR_CONTAINS(out, hms::HmsClient::kLegacyKuduStorageHandler);
+  ASSERT_STR_CONTAINS(out, "default.a");
 
-    // Check if the metadata is in sync between the Hive Metastore and Kudu.
-    NO_FATALS(RunActionStdoutString(
-        Substitute("hms check $0 --unlock_experimental_flags=true "
-                   "--hive_metastore_uris=$1", master_addr, cluster_->hms()->uris()),
-        &out));
-    ASSERT_STR_CONTAINS(out, "OK");
+  // The table should still be accessible in both Kudu and the HMS.
+  shared_ptr<KuduTable> kudu_table;
+  ASSERT_OK(kudu_client->OpenTable("default.a", &kudu_table));
+  hive::Table hms_table;
+  ASSERT_OK(hms_client.GetTable("default", "a", &hms_table));
 
-    shared_ptr<KuduTable> kudu_table;
-    ASSERT_OK(kudu_client->OpenTable("default.a", &kudu_table));
-    ASSERT_OK(kudu_client->OpenTable("default.b", &kudu_table));
-    hive::Table hms_table;
-    ASSERT_OK(hms_client.GetTable("default", "a", &hms_table));
-    ASSERT_OK(hms_client.GetTable("default", "b", &hms_table));
-  }
+  // Check that re-upgrading works as expected.
+  RunActionStdoutNone(Substitute("hms fix $0", base_flags));
+  RunActionStdoutNone(Substitute("hms check $0", base_flags));
 }
 
-TEST_F(ToolTest, TestCheckHmsMetadata) {
+// Test HMS inconsistencies that can be automatically fixed.
+TEST_F(ToolTest, TestCheckAndAutomaticFixHmsMetadata) {
   ExternalMiniClusterOptions opts;
   opts.hms_mode = HmsMode::ENABLE_HIVE_METASTORE;
   NO_FATALS(StartExternalMiniCluster(std::move(opts)));
@@ -2372,140 +2224,245 @@ TEST_F(ToolTest, TestCheckHmsMetadata) {
   ASSERT_OK(hms_client.Start());
   ASSERT_TRUE(hms_client.IsConnected());
 
-  string hms_table_id = "table_id";
+  FLAGS_hive_metastore_uris = cluster_->hms()->uris();
+  HmsCatalog hms_catalog(master_addr);
+  ASSERT_OK(hms_catalog.Start());
+
   shared_ptr<KuduClient> kudu_client;
-  ASSERT_OK(KuduClientBuilder()
-      .add_master_server_addr(master_addr)
-      .Build(&kudu_client));
+  ASSERT_OK(cluster_->CreateClient(nullptr, &kudu_client));
+
+  string base_flags = Substitute("$0 --unlock_experimental_flags --hive_metastore_uris=$1",
+                                 master_addr, cluster_->hms()->uris());
+
+  // While the metastore integration is disabled create tables in Kudu and the
+  // HMS with inconsistent metadata.
+
+  // Control case: the check tool should not flag this table.
+  shared_ptr<KuduTable> control;
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.control"));
+  ASSERT_OK(kudu_client->OpenTable("default.control", &control));
+  ASSERT_OK(hms_catalog.CreateTable(
+        control->id(), control->name(),
+        client::SchemaFromKuduSchema(control->schema())));
+
+  // Test case: Upper-case names are handled specially in a few places.
+  shared_ptr<KuduTable> test_uppercase;
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.UPPERCASE"));
+  ASSERT_OK(kudu_client->OpenTable("default.UPPERCASE", &test_uppercase));
+  ASSERT_OK(hms_catalog.CreateTable(
+        test_uppercase->id(), test_uppercase->name(),
+        client::SchemaFromKuduSchema(test_uppercase->schema())));
+
+  // Test case: inconsistent schema.
+  shared_ptr<KuduTable> inconsistent_schema;
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.inconsistent_schema"));
+  ASSERT_OK(kudu_client->OpenTable("default.inconsistent_schema", &inconsistent_schema));
+  ASSERT_OK(hms_catalog.CreateTable(
+        inconsistent_schema->id(), inconsistent_schema->name(),
+        SchemaBuilder().Build()));
+
+  // Test case: inconsistent name.
+  shared_ptr<KuduTable> inconsistent_name;
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.inconsistent_name"));
+  ASSERT_OK(kudu_client->OpenTable("default.inconsistent_name", &inconsistent_name));
+  ASSERT_OK(hms_catalog.CreateTable(
+        inconsistent_name->id(), "default.inconsistent_name_hms",
+        client::SchemaFromKuduSchema(inconsistent_name->schema())));
+
+  // Test case: inconsistent master addresses.
+  shared_ptr<KuduTable> inconsistent_master_addrs;
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.inconsistent_master_addrs"));
+  ASSERT_OK(kudu_client->OpenTable("default.inconsistent_master_addrs",
+        &inconsistent_master_addrs));
+  HmsCatalog invalid_hms_catalog("invalid-master-addrs");
+  ASSERT_OK(invalid_hms_catalog.Start());
+  ASSERT_OK(invalid_hms_catalog.CreateTable(
+        inconsistent_master_addrs->id(), inconsistent_master_addrs->name(),
+        client::SchemaFromKuduSchema(inconsistent_master_addrs->schema())));
+
+  // Test cases: orphan tables in the HMS.
+  ASSERT_OK(hms_catalog.CreateTable(
+        "orphan-hms-table-id", "default.orphan_hms_table",
+        SchemaBuilder().Build()));
+  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "orphan_hms_table_legacy_external",
+        "default.orphan_hms_table_legacy_external",
+        master_addr, HmsClient::kExternalTable));
+  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "orphan_hms_table_legacy_managed",
+        "impala::default.orphan_hms_table_legacy_managed",
+        master_addr, HmsClient::kExternalTable));
+
+  // 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));
+
+  // 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));
+
+  // 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));
+
+  // Test case: Kudu table in non-default database.
+  hive::Database db;
+  db.name = "my_db";
+  ASSERT_OK(hms_client.CreateDatabase(db));
+  ASSERT_OK(CreateKuduTable(kudu_client, "my_db.table"));
+
+  // Enable the HMS integration.
+  cluster_->ShutdownNodes(cluster::ClusterNodes::MASTERS_ONLY);
+  cluster_->EnableMetastoreIntegration();
+  ASSERT_OK(cluster_->Restart());
 
-  // 1. Create a Kudu table in Kudu and a legacy table in the HMS, and check the metadata
-  //    consistency.
-  {
-    ASSERT_OK(CreateKuduTable(kudu_client, "a"));
-    shared_ptr<KuduTable> kudu_table;
-    ASSERT_OK(kudu_client->OpenTable("a", &kudu_table));
-    ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "legacy_table",
-                                   HmsClient::kExternalTable));
+  unordered_set<string> consistent_tables = {
+    "default.control",
+  };
 
-    string out;
-    string err;
-    RunActionStdoutStderrString(Substitute("hms check $0 --unlock_experimental_flags=true "
-                                           "--hive_metastore_uris=$1",
-                                           master_addr, cluster_->hms()->uris()), &out, &err);
-    ASSERT_STR_CONTAINS(err, "metadata check tool discovered inconsistent data");
-    ASSERT_STR_CONTAINS(out, "FAILED");
-    ASSERT_STR_CONTAINS(out, kudu_table->id());
-    ASSERT_STR_CONTAINS(out, kudu_table->name());
-    ASSERT_STR_CONTAINS(out, master_addr);
-    ASSERT_STR_CONTAINS(out, "Found legacy tables in the Hive Metastore");
-    ASSERT_STR_CONTAINS(out, "legacy_table");
-  }
-
-  // 2. Create tables in Kudu and the HMS with inconsistent metadata. Then validate the tool
-  //    can detect the metadata inconsistency.
-  {
-    shared_ptr<KuduTable> kudu_table;
-    hive::Table hms_table;
-
-    // Create a table in Kudu and in the HMS with the same table name
-    // but different table ID.
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.b"));
-    ASSERT_OK(kudu_client->OpenTable("default.b", &kudu_table));
-    string table_id_b = kudu_table->id();
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "b", HmsClient::kExternalTable,
-                             master_addr, hms_table_id));
-    ASSERT_OK(hms_client.GetTable("default", "b", &hms_table));
-
-    // Create a table in Kudu and in the HMS with same table name/ID
-    // but different schema. And a table in the HMS with same table
-    // ID but different name.
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.c"));
-    ASSERT_OK(kudu_client->OpenTable("default.c", &kudu_table));
-    string table_id_c = kudu_table->id();
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "c", HmsClient::kExternalTable,
-                             master_addr, table_id_c));
-    ASSERT_OK(hms_client.GetTable("default", "c", &hms_table));
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "d", HmsClient::kExternalTable,
-                             master_addr, table_id_c));
-    ASSERT_OK(hms_client.GetTable("default", "d", &hms_table));
-
-    // Create a table in Kudu and in the HMS with same table name/ID
-    // but different schema/master address.
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.e"));
-    ASSERT_OK(kudu_client->OpenTable("default.e", &kudu_table));
-    string table_id_e = kudu_table->id();
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "e", HmsClient::kExternalTable,
-                             "dummy_master_addr", table_id_c));
-    ASSERT_OK(hms_client.GetTable("default", "e", &hms_table));
-
-    // Finally, create an orphan table in the HMS.
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "orphan_table",
-                             HmsClient::kExternalTable, master_addr, hms_table_id));
-    ASSERT_OK(hms_client.GetTable("default", "orphan_table", &hms_table));
+  unordered_set<string> inconsistent_tables = {
+    "default.UPPERCASE",
+    "default.inconsistent_schema",
+    "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",
+    "legacy_external_hive_incompatible_name",
+    "my_db.table",
+  };
 
+  // Move a list of tables from the inconsistent set to the consistent set.
+  auto make_consistent = [&] (const vector<string>& tables) {
+    for (const string& table : tables) {
+      ASSERT_EQ(inconsistent_tables.erase(table), 1);
+    }
+    consistent_tables.insert(tables.begin(), tables.end());
+  };
+
+  // Run the HMS check tool and verify that the consistent tables are not
+  // reported, and the inconsistent tables are reported.
+  auto check = [&] () {
     string out;
     string err;
-    RunActionStdoutStderrString(Substitute("hms check $0 --unlock_experimental_flags=true "
-                                           "--hive_metastore_uris=$1", master_addr,
-                                           cluster_->hms()->uris()), &out, &err);
-    ASSERT_STR_CONTAINS(err, "metadata check tool discovered inconsistent data");
-    ASSERT_STR_CONTAINS(out, "FAILED");
-    ASSERT_STR_CONTAINS(out, table_id_b);
-    ASSERT_STR_CONTAINS(out, "default.b");
-    ASSERT_STR_CONTAINS(out, table_id_c);
-    ASSERT_STR_CONTAINS(out, "default.c");
-    ASSERT_STR_CONTAINS(out, "c");
-    ASSERT_STR_CONTAINS(out, "d");
-    ASSERT_STR_CONTAINS(out, table_id_e);
-    ASSERT_STR_CONTAINS(out, "default.e");
-    ASSERT_STR_CONTAINS(out, "e");
-    ASSERT_STR_CONTAINS(out, master_addr);
-
-    // Orphan table is not included.
-    ASSERT_STR_NOT_CONTAINS(out, "orphan_table");
-  }
-
-  // Delete these tables and restart external mini cluster to enable Hive
-  // Metastore integration.
-  ASSERT_OK(kudu_client->DeleteTable("a"));
-  ASSERT_OK(kudu_client->DeleteTable("default.b"));
-  ASSERT_OK(kudu_client->DeleteTable("default.c"));
-  ASSERT_OK(kudu_client->DeleteTable("default.e"));
-  hive::EnvironmentContext env_ctx;
-  ASSERT_OK(hms_client.DropTable("default", "b", env_ctx));
-  ASSERT_OK(hms_client.DropTable("default", "c", env_ctx));
-  ASSERT_OK(hms_client.DropTable("default", "d", env_ctx));
-  ASSERT_OK(hms_client.DropTable("default", "e", env_ctx));
-  ASSERT_OK(hms_client.DropTable("default", "legacy_table", env_ctx));
-  ASSERT_OK(hms_client.DropTable("default", "orphan_table", env_ctx));
-  cluster_->EnableMetastoreIntegration();
-  cluster_->ShutdownNodes(cluster::ClusterNodes::ALL);
-  ASSERT_OK(cluster_->Restart());
+    Status s = RunActionStdoutStderrString(Substitute("hms check $0", base_flags), &out, &err);
+    SCOPED_TRACE(strings::CUnescapeOrDie(out));
+    if (inconsistent_tables.empty()) {
+      ASSERT_OK(s);
+      ASSERT_STR_NOT_CONTAINS(err, "found inconsistencies in the Kudu and HMS catalogs");
+    } else {
+      ASSERT_FALSE(s.ok());
+      ASSERT_STR_CONTAINS(err, "found inconsistencies in the Kudu and HMS catalogs");
+    }
+    for (const string& table : consistent_tables) {
+      ASSERT_STR_NOT_CONTAINS(out, table);
+    }
+    for (const string& table : inconsistent_tables) {
+      ASSERT_STR_CONTAINS(out, table);
+    }
+  };
 
-  // 3. Create a Kudu table with HMS integration enabled (a corresponding table will
-  //    be created in the Hms), and an orphan table in the HMS, the metadata should
-  //    be in sync.
-  {
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.a"));
-    shared_ptr<KuduTable> kudu_table;
-    ASSERT_OK(kudu_client->OpenTable("default.a", &kudu_table));
+  // 'hms check' should point out all of the test-case tables, but not the control tables.
+  NO_FATALS(check());
 
-    hive::Table hms_table;
-    ASSERT_OK(hms_client.GetTable("default", "a", &hms_table));
+  // 'hms fix --dryrun should not change the output of 'hms check'.
+  NO_FATALS(RunActionStdoutNone(
+        Substitute("hms fix $0 --dryrun --drop_orphan_hms_tables", base_flags)));
+  NO_FATALS(check());
+
+  // Drop orphan tables.
+  NO_FATALS(RunActionStdoutNone(
+        Substitute("hms fix $0 --drop_orphan_hms_tables --nocreate_missing_hms_tables "
+                   "--noupgrade_hms_tables --nofix_inconsistent_tables", base_flags)));
+  make_consistent({
+    "default.orphan_hms_table",
+    "default.orphan_hms_table_legacy_external",
+    "default.orphan_hms_table_legacy_managed",
+  });
+  NO_FATALS(check());
 
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "b", HmsClient::kExternalTable,
-                             master_addr, hms_table_id));
-    ASSERT_OK(hms_client.GetTable("default", "b", &hms_table));
+  // Create missing hms tables.
+  NO_FATALS(RunActionStdoutNone(
+        Substitute("hms fix $0 --noupgrade_hms_tables --nofix_inconsistent_tables", base_flags)));
+  make_consistent({
+    "default.kudu_orphan",
+    "my_db.table",
+  });
+  NO_FATALS(check());
 
-    string out;
-    NO_FATALS(RunActionStdoutString(Substitute("hms check $0 --unlock_experimental_flags=true "
-                                               "--hive_metastore_uris=$1",
-                                               master_addr, cluster_->hms()->uris()), &out));
-    ASSERT_STR_CONTAINS(out, "OK");
-  }
+  // Upgrade legacy HMS tables.
+  NO_FATALS(RunActionStdoutNone(
+        Substitute("hms fix $0 --nofix_inconsistent_tables", base_flags)));
+  make_consistent({
+    "default.legacy_external",
+    "default.legacy_managed",
+    "legacy_external_hive_incompatible_name",
+  });
+  NO_FATALS(check());
+
+  // Refresh stale HMS tables.
+  NO_FATALS(RunActionStdoutNone(Substitute("hms fix $0", base_flags)));
+  make_consistent({
+    "default.UPPERCASE",
+    "default.inconsistent_schema",
+    "default.inconsistent_name",
+    "default.inconsistent_master_addrs",
+  });
+  NO_FATALS(check());
+
+  ASSERT_TRUE(inconsistent_tables.empty());
+
+  for (const string& table : {
+    "control",
+    "uppercase",
+    "inconsistent_schema",
+    "inconsistent_name_hms",
+    "inconsistent_master_addrs",
+    "kudu_orphan",
+    "legacy_external",
+    "legacy_managed",
+    "legacy_external_hive_incompatible_name",
+  }) {
+    NO_FATALS(ValidateHmsEntries(&hms_client, kudu_client, "default", table, master_addr));
+  }
+
+  // Validate the tables in the other databases.
+  NO_FATALS(ValidateHmsEntries(&hms_client, kudu_client, "my_db", "table", master_addr));
+
+  vector<string> kudu_tables;
+  kudu_client->ListTables(&kudu_tables);
+  std::sort(kudu_tables.begin(), kudu_tables.end());
+  ASSERT_EQ(kudu_tables, vector<string>({
+    "default.control",
+    "default.inconsistent_master_addrs",
+    "default.inconsistent_name_hms",
+    "default.inconsistent_schema",
+    "default.kudu_orphan",
+    "default.legacy_external",
+    "default.legacy_external_hive_incompatible_name",
+    "default.legacy_managed",
+    "default.uppercase",
+    "my_db.table",
+  }));
 }
 
-TEST_F(ToolTest, TestFixHmsMetadata) {
+// Test HMS inconsistencies that must be manually fixed.
+TEST_F(ToolTest, TestCheckAndManualFixHmsMetadata) {
   ExternalMiniClusterOptions opts;
   opts.hms_mode = HmsMode::ENABLE_HIVE_METASTORE;
   NO_FATALS(StartExternalMiniCluster(std::move(opts)));
@@ -2515,98 +2472,126 @@ TEST_F(ToolTest, TestFixHmsMetadata) {
   ASSERT_OK(hms_client.Start());
   ASSERT_TRUE(hms_client.IsConnected());
 
-  string hms_table_id = "table_id";
-  shared_ptr<KuduClient> kudu_client;
-  ASSERT_OK(KuduClientBuilder()
-                .add_master_server_addr(master_addr)
-                .Build(&kudu_client));
-
-  {
-    // Create a table with the same name/ID but different schema in Kudu and the HMS.
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.a"));
-    shared_ptr<KuduTable> kudu_table;
-    ASSERT_OK(kudu_client->OpenTable("default.a", &kudu_table));
-    hive::Table hms_table;
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "a",
-                             HmsClient::kManagedTable, master_addr, kudu_table->id()));
-    ASSERT_OK(hms_client.GetTable("default", "a", &hms_table));
-
-    // Create a table with the same ID but different name/schema in Kudu and the HMS.
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.b"));
-    ASSERT_OK(kudu_client->OpenTable("default.b", &kudu_table));
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "c",
-                             HmsClient::kManagedTable, master_addr, kudu_table->id()));
-    ASSERT_OK(hms_client.GetTable("default", "c", &hms_table));
+  FLAGS_hive_metastore_uris = cluster_->hms()->uris();
+  HmsCatalog hms_catalog(master_addr);
+  ASSERT_OK(hms_catalog.Start());
 
-    // Create a table in Kudu but not the HMS.
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.d"));
-    ASSERT_OK(kudu_client->OpenTable("default.d", &kudu_table));
-
-    // Create an orphan table in the HMS.
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "e",
-                             HmsClient::kManagedTable, master_addr, hms_table_id));
-    ASSERT_OK(hms_client.GetTable("default", "e", &hms_table));
-
-    // Create multiple tables in the HMS sharing the same table ID .
-    ASSERT_OK(CreateKuduTable(kudu_client, "default.kudu"));
-    ASSERT_OK(kudu_client->OpenTable("default.kudu", &kudu_table));
-    string id = kudu_table->id();
-
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "kudu",
-                             HmsClient::kManagedTable, master_addr, id));
-    ASSERT_OK(hms_client.GetTable("default", "kudu", &hms_table));
-
-    ASSERT_OK(CreateHmsTable(&hms_client, "default", "diff_name",
-                             HmsClient::kManagedTable, master_addr, id));
-    ASSERT_OK(hms_client.GetTable("default", "diff_name", &hms_table));
-  }
-
-  // Restart external mini cluster to enable Hive Metastore integration.
+  shared_ptr<KuduClient> kudu_client;
+  ASSERT_OK(cluster_->CreateClient(nullptr, &kudu_client));
+
+  string base_flags = Substitute("$0 --unlock_experimental_flags --hive_metastore_uris=$1",
+                                 master_addr, cluster_->hms()->uris());
+
+  // While the metastore integration is disabled create tables in Kudu and the
+  // HMS with inconsistent metadata.
+
+  // Test case: Multiple HMS tables pointing to a single Kudu table.
+  shared_ptr<KuduTable> duplicate_hms_tables;
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.duplicate_hms_tables"));
+  ASSERT_OK(kudu_client->OpenTable("default.duplicate_hms_tables", &duplicate_hms_tables));
+  ASSERT_OK(hms_catalog.CreateTable(
+        duplicate_hms_tables->id(), "default.duplicate_hms_tables",
+        client::SchemaFromKuduSchema(duplicate_hms_tables->schema())));
+  ASSERT_OK(hms_catalog.CreateTable(
+        duplicate_hms_tables->id(), "default.duplicate_hms_tables_2",
+        client::SchemaFromKuduSchema(duplicate_hms_tables->schema())));
+  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "duplicate_hms_tables_3",
+        "default.duplicate_hms_tables",
+        master_addr, HmsClient::kExternalTable));
+
+  // Test case: Kudu tables Hive-incompatible names.
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.hive-incompatible-name"));
+  ASSERT_OK(CreateKuduTable(kudu_client, "no_database"));
+
+  // Test case: Kudu table in non-existent database.
+  ASSERT_OK(CreateKuduTable(kudu_client, "non_existent_database.table"));
+
+  // Test case: a legacy table with a Hive name which conflicts with another table in Kudu.
+  ASSERT_OK(CreateLegacyHmsTable(&hms_client, "default", "conflicting_legacy_table",
+        "impala::default.conflicting_legacy_table",
+        master_addr, HmsClient::kManagedTable));
+  ASSERT_OK(CreateKuduTable(kudu_client, "impala::default.conflicting_legacy_table"));
+  ASSERT_OK(CreateKuduTable(kudu_client, "default.conflicting_legacy_table"));
+
+  // Enable the HMS integration.
+  cluster_->ShutdownNodes(cluster::ClusterNodes::MASTERS_ONLY);
   cluster_->EnableMetastoreIntegration();
-  cluster_->ShutdownNodes(cluster::ClusterNodes::ALL);
   ASSERT_OK(cluster_->Restart());
 
-  // 2. Run the fix tool and expect it to fail.
-  {
+  // Run the HMS check tool and verify that the inconsistent tables are reported.
+  auto check = [&] () {
+    string out;
     string err;
-    RunActionStderrString(Substitute("hms fix $0 --unlock_experimental_flags=true "
-                                     "--hive_metastore_uris=$1",
-                                     master_addr, cluster_->hms()->uris()), &err);
-    ASSERT_STR_CONTAINS(err, "Illegal state: error fixing inconsistent metadata: "
-                             "Found more than one tables");
-  }
+    Status s = RunActionStdoutStderrString(Substitute("hms check $0", base_flags), &out, &err);
+    SCOPED_TRACE(strings::CUnescapeOrDie(out));
+    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",
+      "default.conflicting_legacy_table",
+    })) {
+      ASSERT_STR_CONTAINS(out, table);
+    }
+  };
+
+  // Check should recognize this inconsistent tables.
+  NO_FATALS(check());
 
-  // 3. Delete one of the tables that share the same table ID in the HMS and then
-  //    run the fix tool again and expect it to succeed.
+  // Fix should fail, since these are not automatically repairable issues.
   {
-    hive::EnvironmentContext env_ctx;
-    ASSERT_OK(hms_client.DropTable("default", "diff_name", env_ctx));
     string out;
-    NO_FATALS(RunActionStdinStdoutString(Substitute("hms fix $0 --unlock_experimental_flags=true "
-                                                    "--hive_metastore_uris=$1",
-                                                    master_addr, cluster_->hms()->uris()),
-                                         "default.c\n", &out));
+    string err;
+    Status s = RunActionStdoutStderrString(Substitute("hms fix $0", base_flags), &out, &err);
+    SCOPED_TRACE(strings::CUnescapeOrDie(out));
+    ASSERT_FALSE(s.ok());
+  }
 
-    NO_FATALS(RunActionStdoutString(Substitute("hms check $0 --unlock_experimental_flags=true "
-                                               "--hive_metastore_uris=$1",
-                                               master_addr, cluster_->hms()->uris()), &out));
-    ASSERT_STR_CONTAINS(out, "OK");
+  // Check should still fail.
+  NO_FATALS(check());
 
-    shared_ptr<KuduTable> kudu_table;
-    ASSERT_OK(kudu_client->OpenTable("default.a", &kudu_table));
-    hive::Table hms_table;
-    ASSERT_OK(hms_client.GetTable("default", "a", &hms_table));
+  // 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"));
 
-    ASSERT_OK(kudu_client->OpenTable("default.c", &kudu_table));
-    ASSERT_OK(hms_client.GetTable("default", "c", &hms_table));
+  // Rename the incompatible names.
+  NO_FATALS(RunActionStdoutNone(Substitute(
+          "table rename-table --noalter-external-catalogs $0 "
+          "default.hive-incompatible-name default.hive_compatible_name", master_addr)));
+  NO_FATALS(RunActionStdoutNone(Substitute(
+          "table rename-table --noalter-external-catalogs $0 "
+          "no_database default.with_database", master_addr)));
 
-    ASSERT_OK(kudu_client->OpenTable("default.d", &kudu_table));
-    ASSERT_OK(hms_client.GetTable("default", "d", &hms_table));
+  // Create the missing database.
+  hive::Database db;
+  db.name = "non_existent_database";
+  ASSERT_OK(hms_client.CreateDatabase(db));
 
-    Status s = kudu_client->OpenTable("default.e", &kudu_table);
-    ASSERT_TRUE(s.IsNotFound());
-    ASSERT_OK(hms_client.GetTable("default", "e", &hms_table));
-  }
+  // Rename the conflicting table.
+  NO_FATALS(RunActionStdoutNone(Substitute(
+          "table rename-table --noalter-external-catalogs $0 "
+          "default.conflicting_legacy_table default.non_conflicting_legacy_table", master_addr)));
+
+  // Run the automatic fixer to create missing HMS table entries.
+  NO_FATALS(RunActionStdoutNone(Substitute("hms fix $0", base_flags)));
+
+  // Check should now be clean.
+  NO_FATALS(RunActionStdoutNone(Substitute("hms check $0", base_flags)));
+
+  // Ensure the tables are available.
+  vector<string> kudu_tables;
+  kudu_client->ListTables(&kudu_tables);
+  std::sort(kudu_tables.begin(), kudu_tables.end());
+  ASSERT_EQ(kudu_tables, vector<string>({
+    "default.conflicting_legacy_table",
+    "default.duplicate_hms_tables",
+    "default.hive_compatible_name",
+    "default.non_conflicting_legacy_table",
+    "default.with_database",
+    "non_existent_database.table",
+  }));
 }
 
 // This test is parameterized on the serialization mode and Kerberos.


[3/4] kudu git commit: hms-tool: refactor check tool and combine upgrade and fix

Posted by da...@apache.org.
http://git-wip-us.apache.org/repos/asf/kudu/blob/8e40dfdb/src/kudu/tools/tool_action_hms.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/tool_action_hms.cc b/src/kudu/tools/tool_action_hms.cc
index 6273ed2..db90ee3 100644
--- a/src/kudu/tools/tool_action_hms.cc
+++ b/src/kudu/tools/tool_action_hms.cc
@@ -19,6 +19,7 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <type_traits>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -32,21 +33,35 @@
 #include "kudu/client/shared_ptr.h"
 #include "kudu/common/schema.h"
 #include "kudu/gutil/map-util.h"
-#include "kudu/gutil/strings/join.h"
 #include "kudu/gutil/strings/split.h"
 #include "kudu/gutil/strings/substitute.h"
-#include "kudu/gutil/strings/util.h"
 #include "kudu/hms/hive_metastore_types.h"
 #include "kudu/hms/hms_catalog.h"
 #include "kudu/hms/hms_client.h"
 #include "kudu/tools/tool_action.h"
 #include "kudu/tools/tool_action_common.h"
 #include "kudu/util/monotime.h"
+#include "kudu/util/slice.h"
 #include "kudu/util/status.h"
 
-DECLARE_int64(timeout_ms); // defined in tool_action_common
+DECLARE_bool(force);
+DECLARE_int64(timeout_ms);
 DECLARE_string(hive_metastore_uris);
 
+DEFINE_bool(dryrun, false,
+    "Print a message for each fix, but do not make modifications to Kudu or the Hive Metastore.");
+DEFINE_bool(drop_orphan_hms_tables, false,
+    "Drop orphan Hive Metastore tables which refer to non-existent Kudu tables.");
+DEFINE_bool(create_missing_hms_tables, true,
+    "Create a Hive Metastore table for each Kudu table which is missing one.");
+DEFINE_bool(fix_inconsistent_tables, true,
+    "Fix tables whose Kudu and Hive Metastore metadata differ. If the table name is "
+    "different, the table is renamed in Kudu to match the HMS. If the columns "
+    "or other metadata is different the HMS is updated to match Kudu.");
+DEFINE_bool(upgrade_hms_tables, true,
+    "Upgrade Hive Metastore tables from the legacy Impala metadata format to the "
+    "new Kudu metadata format.");
+
 namespace kudu {
 namespace tools {
 
@@ -55,11 +70,11 @@ using client::KuduClientBuilder;
 using client::KuduTable;
 using client::KuduTableAlterer;
 using client::sp::shared_ptr;
-using hms::HmsClient;
 using hms::HmsCatalog;
-using std::cin;
+using hms::HmsClient;
 using std::cout;
 using std::endl;
+using std::make_pair;
 using std::ostream;
 using std::pair;
 using std::string;
@@ -69,143 +84,16 @@ using std::vector;
 using strings::Split;
 using strings::Substitute;
 
-DEFINE_bool(enable_input, true,
-            "Whether to enable user input for renaming tables that have Hive"
-            "incompatible names.");
-
-// The key is the table ID. The value is a pair of a table in Kudu and a
-// vector of tables in the HMS that all share the same table ID.
-typedef unordered_map<string, pair<shared_ptr<KuduTable>, vector<hive::Table>>> TablesMap;
-
-const char* const kDefaultDatabaseArg = "default_database";
-const char* const kDefaultDatabaseArgDesc = "The database that non-Impala Kudu "
-                                            "table should reside in";
-const char* const kInvalidNameError = "is not a valid object name";
-
-// Returns a map that contains all Kudu tables in the HMS. Its key is
-// the Kudu table name and its value is the corresponding HMS table.
-unordered_map<string, hive::Table> RetrieveTablesMap(vector<hive::Table> hms_tables) {
-  unordered_map<string, hive::Table> hms_tables_map;
-  for (auto& hms_table : hms_tables) {
-    const string& storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
-    if (storage_handler == HmsClient::kLegacyKuduStorageHandler) {
-      hms_tables_map.emplace(hms_table.parameters[HmsClient::kLegacyKuduTableNameKey], hms_table);
-    } else if (storage_handler == HmsClient::kKuduStorageHandler) {
-      hms_tables_map.emplace(Substitute("$0.$1", hms_table.dbName, hms_table.tableName), hms_table);
-    }
-  }
-  return hms_tables_map;
-}
-
-string RenameHiveIncompatibleTable(const string& table_name) {
-  cout << Substitute("Table $0 is not hive compatible.", table_name) << endl;
-  cout << "Please input a new table name: ";
-  string new_table_name;
-  getline(cin, new_table_name);
-  return new_table_name;
-}
-
 // Only alter the table in Kudu but not in the Hive Metastore.
-Status AlterKuduTableOnly(KuduClient* kudu_client,
-                          const string& name,
-                          const string& new_name) {
+Status RenameTableInKuduCatalog(KuduClient* kudu_client,
+                                const string& name,
+                                const string& new_name) {
   unique_ptr<KuduTableAlterer> alterer(kudu_client->NewTableAlterer(name));
   SetAlterExternalCatalogs(alterer.get(), false);
   return alterer->RenameTo(new_name)
                 ->Alter();
 }
 
-// Alter legacy tables (which includes non-Impala tables, Impala managed/external
-// tables) to follow the format 'database_name.table_name' in table naming in Kudu.
-// Also, create HMS entries for non-Impala tables.
-//
-// Note that non-Impala tables name should conform to Hive naming standard.
-// Otherwise, the upgrade process will fail.
-Status AlterLegacyKuduTables(KuduClient* kudu_client,
-                             HmsCatalog* hms_catalog,
-                             const string& default_database,
-                             const vector<hive::Table>& hms_tables) {
-  vector<string> table_names;
-  RETURN_NOT_OK(kudu_client->ListTables(&table_names));
-  unordered_map<string, hive::Table> hms_tables_map = RetrieveTablesMap(hms_tables);
-
-  // Take care of the orphan tables in the HMS.
-  for (const auto& hms_table : hms_tables_map) {
-    bool exist;
-    RETURN_NOT_OK(kudu_client->TableExists(hms_table.first, &exist));
-    if (!exist) {
-      // Warn instead of dropping the table in the HMS to avoid breakage for
-      // installations which have multiple Kudu clusters pointed at the same HMS.
-      LOG(WARNING) << Substitute("Found orphan table $0.$1 in the Hive Metastore",
-                                 hms_table.second.dbName, hms_table.second.tableName);
-    }
-  }
-
-  unordered_map<string, Status> failures;
-  for (const auto& table_name : table_names) {
-    hive::Table* hms_table = FindOrNull(hms_tables_map, table_name);
-    shared_ptr<KuduTable> kudu_table;
-    RETURN_NOT_OK(kudu_client->OpenTable(table_name, &kudu_table));
-    Status s;
-    if (hms_table) {
-      // Rename legacy Impala managed tables and external tables to follow the
-      // format 'database_name.table_name'. This is a no-op for non-legacy tables
-      // stored in the HMS.
-      if (hms_table->parameters[HmsClient::kStorageHandlerKey] ==
-          HmsClient::kLegacyKuduStorageHandler) {
-        string new_table_name = Substitute("$0.$1", hms_table->dbName, hms_table->tableName);
-        // Hive-compatible table name implies the table has been upgraded previously and
-        // then downgraded. In this case, we only upgrade the legacy Impala table.
-        if (table_name != new_table_name) {
-          s = AlterKuduTableOnly(kudu_client, table_name, new_table_name);
-          if (s.IsAlreadyPresent()) {
-            s = s.CloneAndPrepend(Substitute("Failed to upgrade legacy Impala table '$0.$1' "
-                                             "(Kudu table name: $2), because a Kudu table with "
-                                             "name '$0.$1' already exists'.", hms_table->dbName,
-                                             hms_table->tableName, table_name));
-          }
-        }
-        if (s.ok()) {
-          s = hms_catalog->UpgradeLegacyImpalaTable(
-                  kudu_table->id(), hms_table->dbName, hms_table->tableName,
-                  client::SchemaFromKuduSchema(kudu_table->schema()));
-        }
-      }
-    } else {
-      // Create the table in the HMS.
-      string new_table_name = Substitute("$0.$1", default_database, table_name);
-      Schema schema = client::SchemaFromKuduSchema(kudu_table->schema());
-      s = hms_catalog->CreateTable(kudu_table->id(), new_table_name, schema);
-      while (!s.ok() && FLAGS_enable_input &&
-             (MatchPattern(s.ToString(), Substitute("*$0*", kInvalidNameError)) ||
-              MatchPattern(s.ToString(), Substitute("*$0*", HmsCatalog::kInvalidTableError)))) {
-        new_table_name = Substitute("$0.$1", default_database,
-                                    RenameHiveIncompatibleTable(table_name));
-        s = hms_catalog->CreateTable(kudu_table->id(), new_table_name, schema);
-      }
-      s = s.AndThen([&] {
-        return AlterKuduTableOnly(kudu_client, table_name, new_table_name);
-      });
-    }
-
-    if (!s.ok()) {
-      failures.emplace(table_name, s);
-    }
-  }
-
-  // Returns the first failure that was seen, if any.
-  if (!failures.empty()) {
-    for (const auto& failure : failures) {
-      LOG(WARNING) << Substitute("Failed to upgrade Kudu table $0, because: ",
-                                 failure.first, failure.second.ToString());
-    }
-
-    return failures.begin()->second;
-  } else {
-    return Status::OK();
-  }
-}
-
 Status Init(const RunnerContext& context,
             shared_ptr<KuduClient>* kudu_client,
             unique_ptr<HmsCatalog>* hms_catalog) {
@@ -226,53 +114,7 @@ Status Init(const RunnerContext& context,
       .Build(kudu_client);
 }
 
-// Upgrade the metadata format of legacy impala managed or external tables
-// in HMS entries, as well as rename the existing tables in Kudu to adapt
-// to the new naming rules.
-//
-// Sample Legacy Hms Entries
-// Managed table
-//  Table(
-//      tableName=customer,
-//      dbName=tpch_1000_kudu,
-//      parameters={
-//          kudu.master_addresses: <master-addr>:7051,
-//          kudu.table_name: impala::tpch_1000_kudu.customer,
-//          storage_handler: com.cloudera.kudu.hive.KuduStorageHandler,
-//      },
-//      tableType=MANAGED_TABLE,
-//  )
-//
-//  External table
-//  Table(
-//      tableName=metrics,
-//      dbName=default,
-//      parameters={
-//          EXTERNAL: TRUE,
-//          kudu.master_addresses: <master-addr>,
-//          kudu.table_name: metrics,
-//          storage_handler: com.cloudera.kudu.hive.KuduStorageHandler,
-//      },
-//      tableType=EXTERNAL_TABLE,
-//  )
-Status HmsUpgrade(const RunnerContext& context) {
-  const string& default_database = FindOrDie(context.required_args,
-                                             kDefaultDatabaseArg);
-  shared_ptr<KuduClient> kudu_client;
-  unique_ptr<HmsCatalog> hms_catalog;
-  Init(context, &kudu_client, &hms_catalog);
-
-  // 1. Identify all Kudu tables in the HMS entries.
-  vector<hive::Table> hms_tables;
-  RETURN_NOT_OK(hms_catalog->GetKuduTables(&hms_tables));
-
-  // 2. Rename all existing Kudu tables to have Hive-compatible table names.
-  //    Also, correct all out of sync metadata in HMS entries.
-  return AlterLegacyKuduTables(kudu_client.get(), hms_catalog.get(),
-                               default_database, hms_tables);
-}
-
-// TODO: check that the HMS integration isn't enabled before running it.
+// TODO(dan): check that the HMS integration isn't enabled before running it.
 Status HmsDowngrade(const RunnerContext& context) {
   shared_ptr<KuduClient> kudu_client;
   unique_ptr<HmsCatalog> hms_catalog;
@@ -304,309 +146,523 @@ bool IsSynced(const string& master_addresses,
   return s.ok() && hms_table_copy == hms_table;
 }
 
-// Filter orphan tables from the unsynchronized tables map.
-void FilterOrphanedTables(TablesMap* tables_map) {
-  for (auto it = tables_map->cbegin(); it != tables_map->cend();) {
-    // If the kudu table is empty, then these table in the HMS are
-    // orphan tables. Filter it as we do not care about orphan tables.
-    if (it->second.first == nullptr) {
-      for (const auto &table : it->second.second) {
-        LOG(WARNING) << Substitute("Found orphan table $0.$1 in the Hive Metastore",
-                                   table.dbName, table.tableName);
-      }
-      it = tables_map->erase(it);
-    } else {
-      ++it;
-    }
+// Prints catalog information about Kudu tables in data table format to 'out'.
+Status PrintKuduTables(const string& master_addrs,
+                       const vector<shared_ptr<KuduTable>>& kudu_tables,
+                       ostream& out) {
+  DataTable table({
+      "Kudu table",
+      "Kudu table ID",
+      "Kudu master addresses",
+  });
+  for (const auto& kudu_table : kudu_tables) {
+    table.AddRow({
+        kudu_table->name(),
+        kudu_table->id(),
+        master_addrs,
+    });
   }
+  return table.PrintTo(out);
 }
 
-// Sample of unsynchronized tables:
-//
-// TableID| KuduTableName| HmsDbName| HmsTableName| KuduMasterAddresses| HmsTableMasterAddresses
-//--------+--------------+----------+-------------+--------------------+-------------------------
-//    1   | a            |          |             | 127.0.0.1:50232    |
-//    2   | default.c    | default  | c           | 127.0.0.1:50232    | 127.0.0.1:50232
-//    2   |              | default  | d           | 127.0.0.1:50232    | 127.0.0.1:50232
-//    3   | default.b    |          |             | 127.0.0.1:50232    |
-Status PrintUnsyncedTables(const string& master_addresses,
-                           const TablesMap& tables_map,
-                           ostream& out) {
-  out << "Metadata of unsynchronized tables:" << endl;
-  DataTable table({ "TableID", "KuduTableName", "HmsDbName", "HmsTableName",
-                    "KuduMasterAddresses", "HmsTableMasterAddresses"});
-  for (const auto& entry : tables_map) {
-    const string& table_id = entry.first;
-    const KuduTable& kudu_table = *entry.second.first.get();
-    const vector<hive::Table>& hms_tables = entry.second.second;
-    const string& kudu_table_name = kudu_table.name();
-    if (hms_tables.empty()) {
-      table.AddRow({ table_id, kudu_table_name, "", "", master_addresses, "" });
+// Prints catalog information about Kudu and HMS tables in data table format to 'out'.
+Status PrintTables(const string& master_addrs,
+                   vector<pair<shared_ptr<KuduTable>, hive::Table*>> tables,
+                   ostream& out) {
+  DataTable table({
+      "Kudu table",
+      "Kudu table ID",
+      "Kudu master addresses",
+      "HMS database",
+      "HMS table",
+      Substitute("HMS $0", HmsClient::kStorageHandlerKey),
+      Substitute("HMS $0", HmsClient::kLegacyKuduTableNameKey),
+      Substitute("HMS $0", HmsClient::kKuduTableIdKey),
+      Substitute("HMS $0", HmsClient::kKuduMasterAddrsKey),
+  });
+  for (auto& pair : tables) {
+    vector<string> row;
+    if (pair.first) {
+      const KuduTable& kudu_table = *pair.first;
+      row.emplace_back(kudu_table.name());
+      row.emplace_back(kudu_table.id());
+      row.emplace_back(master_addrs);
     } else {
-      for (const hive::Table& hms_table : hms_tables) {
-        table.AddRow({
-            table_id,
-            kudu_table_name,
-            hms_table.dbName,
-            hms_table.tableName,
-            master_addresses,
-            FindOrDie(hms_table.parameters, HmsClient::kKuduMasterAddrsKey),
-        });
-      }
+      row.resize(3);
     }
+    if (pair.second) {
+      hive::Table& hms_table = *pair.second;
+      row.emplace_back(hms_table.dbName);
+      row.emplace_back(hms_table.tableName);
+      row.emplace_back(hms_table.parameters[HmsClient::kStorageHandlerKey]);
+      row.emplace_back(hms_table.parameters[HmsClient::kLegacyKuduTableNameKey]);
+      row.emplace_back(hms_table.parameters[HmsClient::kKuduTableIdKey]);
+      row.emplace_back(hms_table.parameters[HmsClient::kKuduMasterAddrsKey]);
+    } else {
+      row.resize(9);
+    }
+    table.AddRow(std::move(row));
   }
-
   return table.PrintTo(out);
 }
 
-Status PrintLegacyTables(const vector<hive::Table>& tables, ostream& out) {
-  cout << "Found legacy tables in the Hive Metastore, "
-       << "use metadata upgrade tool first: 'kudu hms upgrade'."
-       << endl;
-  DataTable table({ "HmsDbName", "HmsTableName", "KuduTableName",
-                    "KuduMasterAddresses"});
-  for (hive::Table t : tables) {
-    const string& kudu_table_name = t.parameters[HmsClient::kLegacyKuduTableNameKey];
-    const string& master_addresses = t.parameters[HmsClient::kKuduMasterAddrsKey];
-    table.AddRow({ t.dbName, t.tableName, kudu_table_name, master_addresses });
+// A report of inconsistent tables in Kudu and the HMS catalogs.
+struct CatalogReport {
+  // Kudu tables in the HMS catalog which have no corresponding table in the
+  // Kudu catalog (including legacy tables).
+  vector<hive::Table> orphan_hms_tables;
+
+  // Tables in the Kudu catalog which have no corresponding table in the HMS catalog.
+  vector<shared_ptr<KuduTable>> missing_hms_tables;
+
+  // Tables in the Kudu catalog which have a Hive-incompatible name, and which
+  // are not referenced by an existing legacy Hive table (otherwise they would
+  // fall in to 'inconsistent_tables').
+  //
+  // These tables can not be automatically corrected by the fix tool.
+  vector<shared_ptr<KuduTable>> invalid_name_tables;
+
+  // Legacy Imapala/Kudu tables (storage handler is com.cloudera.kudu.hive.KuduStorageHandler).
+  vector<pair<shared_ptr<KuduTable>, hive::Table>> legacy_hms_tables;
+
+  // Kudu tables with multiple HMS table entries. The entries may or may not be legacy.
+  //
+  // These tables can not be automatically corrected by the fix tool.
+  vector<pair<shared_ptr<KuduTable>, hive::Table>> duplicate_hms_tables;
+
+  // Tables whose Kudu catalog table and HMS table are inconsistent.
+  vector<pair<shared_ptr<KuduTable>, hive::Table>> inconsistent_tables;
+
+  // Returns true if the report is empty.
+  bool empty() const {
+    return orphan_hms_tables.empty()
+      && legacy_hms_tables.empty()
+      && duplicate_hms_tables.empty()
+      && inconsistent_tables.empty()
+      && missing_hms_tables.empty()
+      && invalid_name_tables.empty();
   }
-  return table.PrintTo(out);
-}
-
-Status RetrieveUnsyncedTables(HmsCatalog* hms_catalog,
-                              KuduClient* kudu_client,
-                              const string& master_addresses,
-                              TablesMap* unsynced_tables_map,
-                              vector<hive::Table>* legacy_tables) {
-  // 1. Identify all Kudu table in the HMS entries.
-  vector<hive::Table> hms_tables;
-  RETURN_NOT_OK(hms_catalog->GetKuduTables(&hms_tables));
+};
 
-  // 2. Walk through all the Kudu tables in the HMS and identify any
-  //    out of sync tables.
-  for (auto& 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];
-      string table_name = Substitute("$0.$1", hms_table.dbName, hms_table.tableName);
+// Retrieves the entire Kudu catalog, as well as all Kudu tables in the HMS
+// catalog, and compares them to find inconsistencies.
+//
+// Inconsistencies are bucketed into different groups, corresponding to how they
+// can be repaired.
+Status AnalyzeCatalogs(const string& master_addrs,
+                       HmsCatalog* hms_catalog,
+                       KuduClient* kudu_client,
+                       CatalogReport* report) {
+  // Step 1: retrieve all Kudu tables, and aggregate them by ID and by name. The
+  // by-ID map will be used to match the HMS Kudu table entries. The by-name map
+  // will be used to match against legacy Impala/Kudu HMS table entries.
+  unordered_map<string, shared_ptr<KuduTable>> kudu_tables_by_id;
+  unordered_map<string, shared_ptr<KuduTable>> kudu_tables_by_name;
+  {
+    vector<string> kudu_table_names;
+    RETURN_NOT_OK(kudu_client->ListTables(&kudu_table_names));
+    for (const string& kudu_table_name : kudu_table_names) {
       shared_ptr<KuduTable> kudu_table;
-      Status s = kudu_client->OpenTable(table_name, &kudu_table);
-      if (s.ok() && !IsSynced(master_addresses, *kudu_table.get(), hms_table)) {
-        (*unsynced_tables_map)[kudu_table->id()].first = kudu_table;
-        (*unsynced_tables_map)[hms_table_id].second.emplace_back(hms_table);
-      } else if (s.IsNotFound()) {
-        // We cannot determine whether this table is an orphan table in the HMS now, since
-        // there may be other tables in Kudu shares the same table ID but not the same name.
-        // So do it in the filtering step below.
-        (*unsynced_tables_map)[hms_table_id].second.emplace_back(hms_table);
-      } else {
-        RETURN_NOT_OK(s);
-      }
-    } else if (storage_handler == HmsClient::kLegacyKuduStorageHandler) {
-      legacy_tables->push_back(hms_table);
+      // TODO(dan): When the error is NotFound, prepend an admonishment about not
+      // running this tool when the catalog is in-flux.
+      RETURN_NOT_OK(kudu_client->OpenTable(kudu_table_name, &kudu_table));
+      kudu_tables_by_id.emplace(kudu_table->id(), kudu_table);
+      kudu_tables_by_name.emplace(kudu_table->name(), std::move(kudu_table));
     }
   }
 
-  // 3. If any Kudu table is not present in the HMS, consider it as an out of sync
-  //    table.
-  vector<string> table_names;
-  RETURN_NOT_OK(kudu_client->ListTables(&table_names));
-  unordered_map<string, hive::Table> hms_tables_map = RetrieveTablesMap(std::move(hms_tables));
-  for (const auto& table_name : table_names) {
-    if (!ContainsKey(hms_tables_map, table_name)) {
-      shared_ptr<KuduTable> kudu_table;
-      RETURN_NOT_OK(kudu_client->OpenTable(table_name, &kudu_table));
-      (*unsynced_tables_map)[kudu_table->id()].first = kudu_table;
+  // 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;
+  {
+    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);
+        if (kudu_table) {
+          hms_tables_by_id[(*kudu_table)->id()].emplace_back(std::move(hms_table));
+        } else {
+          orphan_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);
+        if (kudu_table) {
+          hms_tables_by_id[(*kudu_table)->id()].emplace_back(std::move(hms_table));
+        } else {
+          orphan_tables.emplace_back(std::move(hms_table));
+        }
+      }
     }
   }
 
-  // 4. Filter orphan tables.
-  FilterOrphanedTables(unsynced_tables_map);
+  // Step 3: Determine the state of each Kudu table's HMS entry(ies), and bin
+  // them appropriately.
+  vector<pair<shared_ptr<KuduTable>, hive::Table>> legacy_tables;
+  vector<pair<shared_ptr<KuduTable>, hive::Table>> duplicate_tables;
+  vector<pair<shared_ptr<KuduTable>, hive::Table>> stale_tables;
+  vector<shared_ptr<KuduTable>> missing_tables;
+  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);
+
+    if (!hms_tables) {
+      const string& table_name = kudu_table->name();
+      string normalized_table_name(table_name.data(), table_name.size());
+      Status s = hms::HmsCatalog::NormalizeTableName(&normalized_table_name);
+      if (!s.ok()) {
+        invalid_name_tables.emplace_back(std::move(kudu_table));
+      } else {
+        missing_tables.emplace_back(std::move(kudu_table));
+      }
+    } else if (hms_tables->size() == 1) {
+      hive::Table& hms_table = (*hms_tables)[0];
+      const string& storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
+      if (storage_handler == HmsClient::kKuduStorageHandler &&
+          !IsSynced(master_addrs, *kudu_table, hms_table)) {
+        stale_tables.emplace_back(make_pair(std::move(kudu_table), std::move(hms_table)));
+      } else if (storage_handler == HmsClient::kLegacyKuduStorageHandler) {
+        legacy_tables.emplace_back(make_pair(std::move(kudu_table), std::move(hms_table)));
+      }
+    } else {
+      for (hive::Table& hms_table : *hms_tables) {
+        duplicate_tables.emplace_back(make_pair(kudu_table, std::move(hms_table)));
+      }
+    }
+  }
 
+  report->orphan_hms_tables.swap(orphan_tables);
+  report->missing_hms_tables.swap(missing_tables);
+  report->invalid_name_tables.swap(invalid_name_tables);
+  report->legacy_hms_tables.swap(legacy_tables);
+  report->duplicate_hms_tables.swap(duplicate_tables);
+  report->inconsistent_tables.swap(stale_tables);
   return Status::OK();
 }
 
 Status CheckHmsMetadata(const RunnerContext& context) {
-  const string& master_addresses = FindOrDie(context.required_args, kMasterAddressesArg);
+  const string& master_addrs = FindOrDie(context.required_args, kMasterAddressesArg);
   shared_ptr<KuduClient> kudu_client;
   unique_ptr<HmsCatalog> hms_catalog;
-  Init(context, &kudu_client, &hms_catalog);
+  RETURN_NOT_OK(Init(context, &kudu_client, &hms_catalog));
 
-  TablesMap unsynced_tables_map;
-  std::vector<hive::Table> legacy_tables;
-  RETURN_NOT_OK_PREPEND(RetrieveUnsyncedTables(hms_catalog.get(),
-                                               kudu_client.get(),
-                                               master_addresses,
-                                               &unsynced_tables_map,
-                                               &legacy_tables),
-                        "error fetching unsynchronized tables");
-
-  // All good.
-  if (unsynced_tables_map.empty() && legacy_tables.empty()) {
-    cout << "OK" << endl;
+  CatalogReport report;
+  RETURN_NOT_OK(AnalyzeCatalogs(master_addrs, hms_catalog.get(), kudu_client.get(), &report));
+
+  if (report.empty()) {
     return Status::OK();
   }
 
-  // Something went wrong.
-  cout << "FAILED" << endl;
-  if (!unsynced_tables_map.empty()) {
-    RETURN_NOT_OK_PREPEND(PrintUnsyncedTables(master_addresses, unsynced_tables_map, cout),
-                          "error printing inconsistent data");
+  if (!report.invalid_name_tables.empty()) {
+    cout << "Found Kudu table(s) with Hive-incompatible names:" << endl;
+    RETURN_NOT_OK(PrintKuduTables(master_addrs, report.invalid_name_tables, cout));
+    cout << endl
+         << "Suggestion: rename the Kudu table(s) to be Hive-compatible, then run the fix tool:"
+         << endl;
+    for (const auto& table : report.invalid_name_tables) {
+      cout << "\t$ kudu table rename_table --alter_external_catalogs=false "
+           << master_addrs << " " << table->name() << " <database-name>.<table-name>" << endl;
+    }
     cout << endl;
   }
 
-  if (!legacy_tables.empty()) {
-    RETURN_NOT_OK_PREPEND(PrintLegacyTables(legacy_tables, cout),
-                          "error printing legacy tables");
+  if (!report.duplicate_hms_tables.empty()) {
+    vector<pair<shared_ptr<KuduTable>, hive::Table*>> tables;
+    for (auto& table : report.duplicate_hms_tables) {
+      tables.emplace_back(table.first, &table.second);
+    }
+    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 "
+         << endl
+         << "tables and consider recreating them as views referencing the base Kudu table."
+         << endl
+         << endl;
   }
 
-  return Status::RuntimeError("metadata check tool discovered inconsistent data");
-}
-
-Status FixUnsyncedTables(KuduClient* kudu_client,
-                         HmsCatalog* hms_catalog,
-                         const TablesMap& tables_map) {
-  for (const auto& entry : tables_map) {
-    const KuduTable& kudu_table = *entry.second.first;
-    const vector<hive::Table>& hms_tables = entry.second.second;
-
-    // 1. Create the table in the HMS if there is no corresponding table there.
-    string table_id = entry.first;
-    string kudu_table_name = kudu_table.name();
-    Schema schema = client::SchemaFromKuduSchema(kudu_table.schema());
-    cout << Substitute("Table (ID $0) is out of sync.", table_id) << endl;
-    if (hms_tables.empty()) {
-      RETURN_NOT_OK(hms_catalog->CreateTable(table_id, kudu_table_name,  schema));
-      continue;
+  if (!report.orphan_hms_tables.empty() || !report.missing_hms_tables.empty()
+      || !report.legacy_hms_tables.empty() || !report.inconsistent_tables.empty()) {
+    vector<pair<shared_ptr<KuduTable>, hive::Table*>> tables;
+    for (const auto& kudu_table : report.missing_hms_tables) {
+      tables.emplace_back(kudu_table, nullptr);
     }
-
-    // 2. If more than one table shares the same table ID in the HMS, return an error
-    //    as it is unsafe to do an automated fix.
-    //
-    if (hms_tables.size() > 1) {
-      auto table_to_string = [] (const hive::Table& hms_table) {
-          return strings::Substitute("$0.$1", hms_table.dbName, hms_table.tableName);
-      };
-      return Status::IllegalState(
-          Substitute("Found more than one tables [$0] in the Hive Metastore, with the "
-                     "same table ID: $1", JoinMapped(hms_tables, table_to_string, ", "),
-                     table_id));
+    for (auto& table : report.legacy_hms_tables) {
+      tables.emplace_back(table.first, &table.second);
     }
-
-    // 3. If the table name in Kudu is different from the one in the HMS, correct the
-    //    table name in Kudu with the one in the HMS. Since we consider the HMS as the
-    //    source of truth for table names.
-    hive::Table hms_table = hms_tables[0];
-    string hms_table_name = Substitute("$0.$1", hms_table.dbName, hms_table.tableName);
-    if (kudu_table_name != hms_table_name) {
-      string new_table_name;
-      cout << Substitute("Renaming Kudu table $0 [id=$1] to $2 to match the Hive "
-                         "Metastore catalog.", kudu_table_name, table_id, hms_table_name)
-           << endl;
-      RETURN_NOT_OK(AlterKuduTableOnly(kudu_client, kudu_table_name, hms_table_name));
-      kudu_table_name = hms_table_name;
+    for (auto& table : report.inconsistent_tables) {
+      tables.emplace_back(table.first, &table.second);
+    }
+    for (auto& hms_table : report.orphan_hms_tables) {
+      tables.emplace_back(shared_ptr<KuduTable>(), &hms_table);
+    }
+    cout << "Found table(s) with missing or inconsisentent metadata in the Kudu "
+            "catalog or Hive Metastore:" << endl;
+    RETURN_NOT_OK(PrintTables(master_addrs, std::move(tables), cout));
+
+    if (report.orphan_hms_tables.empty()) {
+      cout << endl
+          << "Suggestion: use the fix tool to repair the Kudu and Hive Metastore catalog metadata:"
+          << endl
+          << "\t$ kudu hms fix " << master_addrs << endl;
+    } else {
+      cout << endl
+          << "Suggestion: use the fix tool to repair the Kudu and Hive Metastore catalog metadata"
+          << endl
+          << "and drop Hive Metastore table(s) which reference non-existent Kudu tables:" << endl
+          << "\t$ kudu hms fix --drop_orphan_hms_tables " << master_addrs << endl;
     }
-
-    // 4. Correct the master addresses, and the column name and type based on the
-    //    information in Kudu.
-    cout << Substitute("Updating metadata of table $0 [id=$1] in Hive Metastore catalog.",
-                       kudu_table_name, table_id) << endl;
-    RETURN_NOT_OK(hms_catalog->AlterTable(table_id, hms_table_name,
-                                          hms_table_name, schema));
   }
 
-  return Status::OK();
+  // TODO(dan): add a link to the HMS guide on kudu.apache.org to this message.
+  return Status::IllegalState("found inconsistencies in the Kudu and HMS catalogs");
 }
 
+// Pretty-prints the table name and ID.
+string TableIdent(const KuduTable& table) {
+  return Substitute("$0 [id=$1]", table.name(), table.id());
+}
+
+// Analyzes the Kudu and HMS catalogs and attempts to fix any
+// automatically-fixable issues.
+//
+// Error handling: unexpected errors (e.g. networking errors) are fatal and
+// result in returning early. Expected application failures such as a rename
+// failing due to duplicate table being present are logged and execution
+// continues.
 Status FixHmsMetadata(const RunnerContext& context) {
-  const string& master_addresses = FindOrDie(context.required_args, kMasterAddressesArg);
+  const string& master_addrs = FindOrDie(context.required_args, kMasterAddressesArg);
   shared_ptr<KuduClient> kudu_client;
   unique_ptr<HmsCatalog> hms_catalog;
-  Init(context, &kudu_client, &hms_catalog);
+  RETURN_NOT_OK(Init(context, &kudu_client, &hms_catalog));
 
-  TablesMap unsynced_tables_map;
-  std::vector<hive::Table> legacy_tables;
-  RETURN_NOT_OK_PREPEND(RetrieveUnsyncedTables(hms_catalog.get(),
-                                               kudu_client.get(),
-                                               master_addresses,
-                                               &unsynced_tables_map,
-                                               &legacy_tables),
-                        "error fetching unsynchronized tables");
-
-  if (unsynced_tables_map.empty() && legacy_tables.empty()) {
-    cout << "Metadata between Kudu and Hive Metastore is in sync." << endl;
-    return Status::OK();
+  CatalogReport report;
+  RETURN_NOT_OK(AnalyzeCatalogs(master_addrs, hms_catalog.get(), kudu_client.get(), &report));
+
+  bool success = true;
+
+  if (FLAGS_drop_orphan_hms_tables) {
+    for (hive::Table& hms_table : report.orphan_hms_tables) {
+      string table_name = Substitute("$0.$1", hms_table.dbName, hms_table.tableName);
+      const string& master_addrs_param = hms_table.parameters[HmsClient::kKuduMasterAddrsKey];
+      if (master_addrs_param != master_addrs && !FLAGS_force) {
+        LOG(INFO) << "Skipping drop of orphan HMS table " << table_name
+                  << " with master addresses parameter " << master_addrs_param
+                  << " because it does not match the --" << kMasterAddressesArg << " argument"
+                  << " (use --force to skip this check)";
+        continue;
+      }
+
+      if (FLAGS_dryrun) {
+        LOG(INFO) << "[dryrun] Dropping orphan HMS table " << table_name;
+      } else {
+        const string& table_id = hms_table.parameters[HmsClient::kKuduTableIdKey];
+        const string& storage_handler = hms_table.parameters[HmsClient::kStorageHandlerKey];
+        // All errors are fatal here, since we've already checked that the table exists in the HMS.
+        if (storage_handler == HmsClient::kKuduStorageHandler) {
+          RETURN_NOT_OK_PREPEND(hms_catalog->DropTable(table_id, table_name),
+              Substitute("failed to drop orphan HMS table $0", table_name));
+        } else {
+          RETURN_NOT_OK_PREPEND(hms_catalog->DropLegacyTable(table_name),
+              Substitute("failed to drop legacy orphan HMS table $0", table_name));
+        }
+      }
+    }
   }
 
-  // print the legacy tables if any, and returns a runtime error.
-  if (!legacy_tables.empty()) {
-    RETURN_NOT_OK_PREPEND(PrintLegacyTables(legacy_tables, cout),
-                          "error printing legacy tables");
-    return Status::RuntimeError("metadata fix tool encountered fatal errors");
+  if (FLAGS_create_missing_hms_tables) {
+    for (const auto& kudu_table : report.missing_hms_tables) {
+      const string& table_id = kudu_table->id();
+      const string& table_name = kudu_table->name();
+      Schema schema = client::SchemaFromKuduSchema(kudu_table->schema());
+      string normalized_table_name(table_name.data(), table_name.size());
+      CHECK_OK(hms::HmsCatalog::NormalizeTableName(&normalized_table_name));
+
+      if (FLAGS_dryrun) {
+        LOG(INFO) << "[dryrun] Creating HMS table for Kudu table " << TableIdent(*kudu_table);
+      } else {
+        Status s = hms_catalog->CreateTable(table_id, table_name, schema);
+        if (s.IsAlreadyPresent()) {
+          LOG(ERROR) << "Failed to create HMS table for Kudu table "
+                     << TableIdent(*kudu_table)
+                     << " because another table already exists in the HMS with that name";
+          success = false;
+          continue;
+        }
+        if (s.IsInvalidArgument()) {
+          // This most likely means the database doesn't exist, but it is ambiguous.
+          LOG(ERROR) << "Failed to create HMS table for Kudu table "
+                     << TableIdent(*kudu_table)
+                     << " (database does not exist?): " << s.message().ToString();
+          success = false;
+          continue;
+        }
+        // All other errors are unexpected.
+        RETURN_NOT_OK_PREPEND(s,
+            Substitute("failed to create HMS table for Kudu table $0", TableIdent(*kudu_table)));
+      }
+
+      if (normalized_table_name != table_name) {
+        if (FLAGS_dryrun) {
+          LOG(INFO) << "[dryrun] Renaming Kudu table " << TableIdent(*kudu_table)
+                    << " to lowercased Hive-compatible name: " << normalized_table_name;
+        } else {
+          // All errors are fatal. We never expect to get an 'AlreadyPresent'
+          // error, since the catalog manager validates that no two
+          // Hive-compatible table names differ only by case.
+          //
+          // Note that if an error occurs we do not roll-back the HMS table
+          // creation step, since a subsequent run of the tool will recognize
+          // the table as an inconsistent table (Kudu and HMS table names do not
+          // match), and automatically fix it.
+          RETURN_NOT_OK_PREPEND(
+              RenameTableInKuduCatalog(kudu_client.get(), table_name, normalized_table_name),
+              Substitute("failed to rename Kudu table $0 to lowercased Hive compatible name $1",
+                         TableIdent(*kudu_table), normalized_table_name));
+        }
+      }
+    }
   }
 
-  // Fix inconsistent metadata.
-  RETURN_NOT_OK_PREPEND(FixUnsyncedTables(kudu_client.get(), hms_catalog.get(),
-                                          unsynced_tables_map),
-                        "error fixing inconsistent metadata");
-  cout << "DONE" << endl;
-  return Status::OK();
+  if (FLAGS_upgrade_hms_tables) {
+    for (const auto& table_pair : report.legacy_hms_tables) {
+      const KuduTable& kudu_table = *table_pair.first;
+      const hive::Table& hms_table = table_pair.second;
+      string hms_table_name = Substitute("$0.$1", hms_table.dbName, hms_table.tableName);
+
+      if (FLAGS_dryrun) {
+        LOG(INFO) << "[dryrun] Upgrading legacy Impala HMS metadata for table "
+                  << hms_table_name;
+      } else {
+        RETURN_NOT_OK_PREPEND(hms_catalog->UpgradeLegacyImpalaTable(
+                  kudu_table.id(), hms_table.dbName, hms_table.tableName,
+                  client::SchemaFromKuduSchema(kudu_table.schema())),
+            Substitute("failed to upgrade legacy Impala HMS metadata for table $0",
+              hms_table_name));
+      }
+
+      if (kudu_table.name() != hms_table_name) {
+        if (FLAGS_dryrun) {
+          LOG(INFO) << "[dryrun] Renaming Kudu table " << TableIdent(kudu_table)
+                    << " to " << hms_table_name;
+        } else {
+          Status s = RenameTableInKuduCatalog(kudu_client.get(), kudu_table.name(), hms_table_name);
+          if (s.IsAlreadyPresent()) {
+            LOG(ERROR) << "Failed to rename Kudu table " << TableIdent(kudu_table)
+                       << " to match the Hive Metastore name " << hms_table_name
+                       << ", because a Kudu table with name" << hms_table_name
+                       << " already exists";
+            LOG(INFO) << "Suggestion: rename the conflicting table name manually:\n"
+                      << "\t$ kudu table rename_table --alter_external_catalogs=false "
+                      << master_addrs << " " << hms_table_name << " <database-name>.<table-name>'";
+            success = false;
+            continue;
+          }
+
+          // All other errors are fatal. Note that if an error occurs we do not
+          // roll-back the HMS legacy upgrade step, since a subsequent run of
+          // the tool will recognize the table as an inconsistent table (Kudu
+          // and HMS table names do not match), and automatically fix it.
+          RETURN_NOT_OK_PREPEND(s,
+              Substitute("failed to rename Kudu table $0 to $1",
+                TableIdent(kudu_table), hms_table_name));
+        }
+      }
+    }
+  }
+
+  if (FLAGS_fix_inconsistent_tables) {
+    for (const auto& table_pair : report.inconsistent_tables) {
+      const KuduTable& kudu_table = *table_pair.first;
+      const hive::Table& hms_table = table_pair.second;
+      string hms_table_name = Substitute("$0.$1", hms_table.dbName, hms_table.tableName);
+
+      if (hms_table_name != kudu_table.name()) {
+        // Update the Kudu table name to match the HMS table name.
+        if (FLAGS_dryrun) {
+          LOG(INFO) << "[dryrun] Renaming Kudu table " << TableIdent(kudu_table)
+                    << " to " << hms_table_name;
+        } else {
+          Status s = RenameTableInKuduCatalog(kudu_client.get(), kudu_table.name(), hms_table_name);
+          if (s.IsAlreadyPresent()) {
+            LOG(ERROR) << "Failed to rename Kudu table " << TableIdent(kudu_table)
+                       << " to match HMS table " << hms_table_name
+                       << ", because a Kudu table with name " << hms_table_name
+                       << " already exists";
+            success = false;
+            continue;
+          }
+          RETURN_NOT_OK_PREPEND(s,
+              Substitute("failed to rename Kudu table $0 to $1",
+                TableIdent(kudu_table), hms_table_name));
+        }
+      }
+
+      // Update the HMS table metadata to match Kudu.
+      if (FLAGS_dryrun) {
+        LOG(INFO) << "[dryrun] Refreshing HMS table metadata for Kudu table "
+                  << TableIdent(kudu_table);
+      } else {
+        Schema schema(client::SchemaFromKuduSchema(kudu_table.schema()));
+        RETURN_NOT_OK_PREPEND(
+            hms_catalog->AlterTable(kudu_table.id(), hms_table_name, hms_table_name, schema),
+            Substitute("failed to refresh HMS table metadata for Kudu table $0",
+              TableIdent(kudu_table)));
+      }
+    }
+  }
+
+  if (FLAGS_dryrun || success) {
+    return Status::OK();
+  }
+  return Status::RuntimeError("Failed to fix some catalog metadata inconsistencies");
 }
 
 unique_ptr<Mode> BuildHmsMode() {
-  unique_ptr<Action> hms_upgrade =
-      ActionBuilder("upgrade", &HmsUpgrade)
-          .Description("Upgrade the legacy metadata for Kudu and the Hive Metastores")
-          .AddRequiredParameter({ kMasterAddressesArg, kMasterAddressesArgDesc })
-          .AddRequiredParameter({ kDefaultDatabaseArg, kDefaultDatabaseArgDesc })
-          .AddOptionalParameter("hive_metastore_uris")
-          .AddOptionalParameter("hive_metastore_sasl_enabled")
-          .AddOptionalParameter("hive_metastore_retry_count")
-          .AddOptionalParameter("hive_metastore_send_timeout")
-          .AddOptionalParameter("hive_metastore_recv_timeout")
-          .AddOptionalParameter("hive_metastore_conn_timeout")
-          .AddOptionalParameter("enable_input")
-          .Build();
 
-  unique_ptr<Action> hms_downgrade =
-      ActionBuilder("downgrade", &HmsDowngrade)
-          .Description("Downgrade the metadata to legacy format for "
-                       "Kudu and the Hive Metastores")
-          .AddRequiredParameter({ kMasterAddressesArg, kMasterAddressesArgDesc })
-          .AddOptionalParameter("hive_metastore_uris")
-          .AddOptionalParameter("hive_metastore_sasl_enabled")
-          .AddOptionalParameter("hive_metastore_retry_count")
-          .AddOptionalParameter("hive_metastore_send_timeout")
-          .AddOptionalParameter("hive_metastore_recv_timeout")
-          .AddOptionalParameter("hive_metastore_conn_timeout")
-          .Build();
+  // TODO(dan): automatically retrieve the HMS URIs and SASL config from the
+  // Kudu master instead of requiring them as an additional flag.
 
   unique_ptr<Action> hms_check =
       ActionBuilder("check", &CheckHmsMetadata)
-          .Description("Check the metadata consistency between Kudu and the Hive Metastores")
+          .Description("Check metadata consistency between Kudu and the Hive Metastore catalogs")
           .AddRequiredParameter({ kMasterAddressesArg, kMasterAddressesArgDesc })
           .AddOptionalParameter("hive_metastore_uris")
           .AddOptionalParameter("hive_metastore_sasl_enabled")
-          .AddOptionalParameter("hive_metastore_retry_count")
-          .AddOptionalParameter("hive_metastore_send_timeout")
-          .AddOptionalParameter("hive_metastore_recv_timeout")
-          .AddOptionalParameter("hive_metastore_conn_timeout")
           .Build();
 
   unique_ptr<Action> hms_fix =
     ActionBuilder("fix", &FixHmsMetadata)
-        .Description("Fix the metadata inconsistency between Kudu and the Hive Metastores")
+        .Description("Fix automatically-repairable metadata inconsistencies in the "
+                     "Kudu and Hive Metastore catalogs")
         .AddRequiredParameter({ kMasterAddressesArg, kMasterAddressesArgDesc })
         .AddOptionalParameter("hive_metastore_uris")
         .AddOptionalParameter("hive_metastore_sasl_enabled")
-        .AddOptionalParameter("hive_metastore_retry_count")
-        .AddOptionalParameter("hive_metastore_send_timeout")
-        .AddOptionalParameter("hive_metastore_recv_timeout")
-        .AddOptionalParameter("hive_metastore_conn_timeout")
+        .AddOptionalParameter("dryrun")
+        .AddOptionalParameter("drop_orphan_hms_tables")
+        .AddOptionalParameter("create_missing_hms_tables")
+        .AddOptionalParameter("fix_inconsistent_tables")
+        .AddOptionalParameter("upgrade_hms_tables")
         .Build();
 
+  // TODO(dan): add 'hms precheck' tool to check for overlapping normalized table names.
+
+  unique_ptr<Action> hms_downgrade =
+      ActionBuilder("downgrade", &HmsDowngrade)
+          .Description("Downgrade the metadata to legacy format for "
+                       "Kudu and the Hive Metastores")
+          .AddRequiredParameter({ kMasterAddressesArg, kMasterAddressesArgDesc })
+          .AddOptionalParameter("hive_metastore_uris")
+          .AddOptionalParameter("hive_metastore_sasl_enabled")
+          .Build();
+
   return ModeBuilder("hms").Description("Operate on remote Hive Metastores")
-                           .AddAction(std::move(hms_upgrade))
                            .AddAction(std::move(hms_downgrade))
                            .AddAction(std::move(hms_check))
                            .AddAction(std::move(hms_fix))