Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 1: (23 comments) http://gerrit.cloudera.org:8080/#/c/10075/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/10075/1/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@174 PS1, Line 174: // Test altering a Kudu (or a historical) table. > nit: I think the term 'legacy' is better than historical, because historica I choose to use 'legacy' here since it is mainly testing that alters table can succeed for all kinds of legacy kudu tables (either managed or external table). http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.h@95 PS1, Line 95: Status RetrieveKuduTablesList(const char* kudu_storage_handler, > As far as I can tell this isn't specific to Kudu at all, it's a generic loo Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.cc@196 PS1, Line 196: hms_tables->emplace(move(table_name), move(hms_table)); > warning: std::move of the const variable 'table_name' has no effect; remove Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_catalog.cc@206 PS1, Line 206: const string& name, > warning: parameter 'name' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_client.h File src/kudu/hms/hms_client.h: http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/hms/hms_client.h@94 PS1, Line 94: static const char* const kKuduTableNameKey; > Are we going to be using this going forward? If not, maybe add 'Deprecated Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/mini-cluster/external_mini_cluster.h@148 PS1, Line 148: bool enable_metastore_integration; > Could you tweak this a bit so there is only one flag to flip in the common Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1854 PS1, Line 1854: // Do not add other parameters such as 'kudu.master_addresses' since > This is problematic because today's HMS table entries that impala creates c Right, but I cannot think of another clean way to get away from the KuduMetastorePlugin metadata checking for onCreateTable event. Besides, I do not feel it is crucial to add 'kudu.master_addresses' here for testing. http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1870 PS1, Line 1870: !MatchPattern(table_name, > Shouldn't this be a strict prefix match? If so HasPrefixString is probably Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1874 PS1, Line 1874: TEST_F(ToolTest, TestHmsUpgrade) { > There are three distinct kinds of tables we need to upgrade: Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1888 PS1, Line 1888: string historical_table_name = StrCat(HmsClient::kHistoricalTablePrefix, > The existing managed table format is 'impala::my_db.my_table'. Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1904 PS1, Line 1904: StrCat(HmsClient::kHistoricalTablePrefix, table_name) > historical_table_name Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/kudu-tool-test.cc@1945 PS1, Line 1945: ASSERT_EQ(database_name, dbname); > If there is only one database and one table, why the for loops? Added more test tables as you suggested. http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@29 PS1, Line 29: #include "kudu/gutil/strings/strcat.h" > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@57 PS1, Line 57: using std::shared_ptr; > warning: using decl 'shared_ptr' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@64 PS1, Line 64: const char* const kMasterAddressesArg = "master_addresses"; > This is defined in tool_action_common.h. Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@66 PS1, Line 66: const char* const kHmsUrisArg = "hms_uris"; > Could you use the existing hive_metastore_uris flag to be consistent? As documented in the TODO, since hive_metastore_uris flag is experimental, it is not possible to use it directly yet. http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@69 PS1, Line 69: bool IsHistoricalTable(const string& table_name) { > Doc this? Kudu doesn't currently have any restrictions on table names, so Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@84 PS1, Line 84: // Renaming existing Impala-managed table name to drop the ‘impala::’ prefix > Looks like this is handling managed tables, but not external tables (where Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@94 PS1, Line 94: kudu_table_name = StrCat(HmsClient::kDefaultDatabaseName, ".", table_name); > What's the reasoning behind having a default database, especially one other Good point. Maybe we should instead ask the users to input which database he/she wants the non-impala Kudu table to reside in? http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@107 PS1, Line 107: vector<string> master_addresses = Split(master_addresses_str, ","); > not used? Used in L123. http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@138 PS1, Line 138: string historical_table_name = StrCat(HmsClient::kHistoricalTablePrefix, > impala::db_name.table_name Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@142 PS1, Line 142: AlterHistoricalTable > Instead of 'historical' everywhere, I think it would be better to name this Done http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@155 PS1, Line 155: // TODO(Hao): update to use AddOptionalParameter once hive_metastore_uris > ah, so we can't have experimental flags be included in tools? Right, experimental flags are checked and have to unlock them with FLAGS_unlock_experimental_flags. -- To view, visit http://gerrit.cloudera.org:8080/10075 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3 Gerrit-Change-Number: 10075 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 17 Apr 2018 03:58:49 +0000 Gerrit-HasComments: Yes
