Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11018 )
Change subject: hms-tool: refactor check tool and combine upgrade and fix ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/11018/5/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/11018/5/src/kudu/hms/hms_catalog.cc@166 PS5, Line 166: Status HmsCatalog::DropLegacyTable(const string& name) { Is there a way to reduce the duplicated code between DropLegacyTable and DropTable? http://gerrit.cloudera.org:8080/#/c/11018/7/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11018/7/src/kudu/tools/kudu-tool-test.cc@2210 PS7, Line 2210: } I think it still worth to enable HMS integration after this and see if check and fix tool works as expected. http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@169 PS6, Line 169: vector<hive::Table>* Why not use const ref? http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@238 PS6, Line 238: duplicate_hms_tables I don't see how duplicate HMS tables are handled in the fix tool. If we expect this to be manually fixed, we should at least log "manual fix is required" in the fix tool? http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@410 PS6, Line 410: drop_orphan_hms_tables Another option to fix the orphan HMS tables (other than dropping the table) is to add the corresponding Kudu tables. Do you think it is worth to add such option in cases that the tables in Kudu were somehow accidentally dropped? http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@451 PS6, Line 451: rename the Kudu table to a Hive-compatible Why not ask user to input the desired name here? http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@474 PS6, Line 474: table_name Should we use normalized table name here? http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@532 PS6, Line 532: UpgradeLegacyImpalaTable I think we cannot simply return early if upgrade HMS tables failed. Since the legacy table name in Kudu has been already renamed. For any kinds of errors we may need to roll back the change in previous step. http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@548 PS6, Line 548: This will : // also overwrite any other stale metadata in the HMS since it's not a : // Kudu-catalog-only alter. I don't see any metadata change in the HMS from L551 to L568. -- To view, visit http://gerrit.cloudera.org:8080/11018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f Gerrit-Change-Number: 11018 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 25 Jul 2018 01:15:46 +0000 Gerrit-HasComments: Yes
