Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 5: (22 comments) http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java File java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@61 PS4, Line 61: LEGACY_KUD > nit: LEGACY Done http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@127 PS4, Line 127: // can only be upgraded to the new format. > This is going to reject any alterations of a legacy Kudu table entry that d Right, I intentionally put it in this way. Because I think this is the right behavior. Once HMS integration is enabled, in Alter table event, the plugin should only allow legacy Kudu table to be upgraded to the new format. I will add a comment to be more explicitly. http://gerrit.cloudera.org:8080/#/c/10075/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@150 PS4, Line 150: > nit: legacy Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@89 PS4, Line 89: Status UpgradeLegacyImpalaTable(const std::string& id, > maybe this should be called something like 'UpgradeLegacyTable'? I think th Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@91 PS4, Line 91: const Schema& schema) WARN_UNUSED_RESULT; > the ordering of the new methods is flipped WRT the .cc Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@95 PS4, Line 95: the c > It's not clear what is 'List' means here. Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.h@96 PS4, Line 96: > Please doc what this key is. Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@183 PS4, Line 183: Status HmsCatalog::UpgradeLegacyImpalaTable(const string& id, > Add a unit test for this method. Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@190 PS4, Line 190: RETURN_NOT_OK(ParseTableName(name, &database_name, &table_name)); > Probably worth adding a 'table_names.clear();' line above this, just to be Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@196 PS4, Line 196: > What happens if you have tables 'a.foo' and 'b.foo', won't this only retain Updated to use kudu.table_name which should be unique. http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/hms/hms_catalog.cc@216 PS4, Line 216: > begin status messages with lowercase. Also maybe reword this to call out t Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/mini-cluster/external_mini_cluster.h@332 PS4, Line 332: SetMetastoreIntegration > Maybe call it EnableMetastoreIntegration in order to be consistent (here an Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@1903 PS4, Line 1903: const shared_ptr<KuduClient>& kudu_client, > alignment Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@1971 PS4, Line 1971: { > Would be good to add test cases for a table which already has a period in t Table name that has a period in the name is not working now. I added a TODO in https://gerrit.cloudera.org/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@79. Because https://github.com/apache/kudu/blob/master/src/kudu/hms/hms_catalog.cc#L509 checks if there are only two parts split by 'dot'. I think we should upgrade that code to allow multiple dots in the table name. How do you think? http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/kudu-tool-test.cc@2007 PS4, Line 2007: external_table_name, master_addr)); > Should the external table entry be validated? Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@73 PS4, Line 73: // tables) to follow the format 'database_name.table_name' in table naming in Kudu. > In general I'm finding it difficult to figure out which kinds of tables are You mean 'vs in HmsCataglog'? Sure, will add more documentation. http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@85 PS4, Line 85: const hive::Table* hms_table = FindOrNull(hms_tables_map, table_name); > Shouldn't this case have already been handled in step 2. below? Step 2 is only upgrading metadata in HMS entries. And here we are upgrading the table name in Kudu. http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@98 PS4, Line 98: return Status::OK(); > We talked yesterday about changing HmsCatalog to not ccreate table entries Hmm, good point. In that case, here maybe we can explicitly call create table in HMS for non impala tables? http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@118 PS4, Line 118: // External table > This indenting is a little funky, I think the parameters should be indented Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@165 PS4, Line 165: entry.second.dbName, entry.second.tableName); > Can be a simple for/each loop? Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@179 PS4, Line 179: > s/legit/Hive-compatible Done http://gerrit.cloudera.org:8080/#/c/10075/4/src/kudu/tools/tool_action_hms.cc@202 PS4, Line 202: > hms Done -- 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: 5 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@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: Sat, 21 Apr 2018 01:14:14 +0000 Gerrit-HasComments: Yes
