Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 16: (9 comments) http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@21 PS16, Line 21: #include <unordered_map> looks like this is no longer needed? Curious that IWYU didn't catch this. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/hms/hms_catalog.h@103 PS16, Line 103: and returns in a vector I'd drop this last part of the sentence, I think it's clear from the signature. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@75 PS16, Line 75: void RetrieveTablesMap(vector<hive::Table> hms_tables, Might be simpler to return the map instead of taking it as an out param. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@121 PS16, Line 121: kudu_client->TableExists(hms_table.first, &exist); Check the return status. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@123 PS16, Line 123: RETURN_NOT_OK(hms_catalog->DropLegacyImpalaTable(hms_table.second.dbName, This could cause problems for installations which have two Kudu clusters pointed at the same HMS. I know we aren't explicitly supporting that for the first cut, but it may be better to warn here instead of dropping, just so things don't break. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@135 PS16, Line 135: for (const auto& table_name : table_names) { What do you think about re-organizing this so that the failure to upgrade a table doesn't prevent all subsequent tables from being upgraded? Probably requires building up a list of Status's which have failed. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@146 PS16, Line 146: string new_table_name = Substitute("$0.$1", hms_table->dbName, hms_table->tableName); Maybe worth doing a check that 'new_table_name' isn't contained in table_names already? Since that would mean a rename conflict. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@148 PS16, Line 148: // If UpgradeLegacyImpalaTable fails, there will be an orphan table in the HMS. Perhaps we could use the notification listener to watch for these upgrade events, and rename the table automatically? That would close most of the race condition here. http://gerrit.cloudera.org:8080/#/c/10075/16/src/kudu/tools/tool_action_hms.cc@156 PS16, Line 156: string new_table_name = Substitute("$0.$1", default_database, As we discussed offline, I think we should just assume the table has already been renamed by this tool. -- 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: 16 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@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: Thu, 17 May 2018 23:31:53 +0000 Gerrit-HasComments: Yes
