Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/hms/hms_client.h File src/kudu/hms/hms_client.h: http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/hms/hms_client.h@90 PS2, Line 90: static const char* const kLegacyKuduStorageHandler; > +1 on legacy over deprecated, maybe extend that to all the old key names? 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: table.tableName = table_name; > Isn't that metadata checking going to be problematic in practice? If it's Yeah, I have changed the metadata checking in AlterTable event. But I do not think it is good to take out the metadata checking in CreateTable event, since legacy tables should not be created anymore once HMS integration is on. Or I am missing something? http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/kudu-tool-test.cc@1897 PS2, Line 1897: void ValidateHmsEntries(HmsClient* hms_client, > I think it'd be simpler to use ASSERTs internally here, and just call it wi Done 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@94 PS1, Line 94: } > Yeah, I think in any case where we're going to rename a table it would be b Sorry, I do not quite follow the last sentence. Not sure how to fill in default database name based on the existing table name? I added a param for users to specify default database name in the tool, is that what you mean 'kudu table name table param'? http://gerrit.cloudera.org:8080/#/c/10075/1/src/kudu/tools/tool_action_hms.cc@155 PS1, Line 155: unordered_map<string, hive::Table> hms_tables_map; > So would this work if the user passes --unlock_experimental_flags? I think Looks like this workaround works. Updated the code. http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/2/src/kudu/tools/tool_action_hms.cc@38 PS2, Line 38: #include "kudu/hms/hms_catalog.h" > warning: #includes are not sorted properly [llvm-include-order] 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: 3 Gerrit-Owner: Hao Hao <[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: Thu, 19 Apr 2018 00:50:17 +0000 Gerrit-HasComments: Yes
