Dan Burkert 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 8: (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: RETURN_NOT_OK(ParseTableName(name, &hms_database, &hms_table)); > Is there a way to reduce the duplicated code between DropLegacyTable and Dr Done 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 chec Done 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? The indexing operations on lines 182-185 require a mutable reference, and our style guide doesn't allow passing by mut ref. http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@238 PS6, Line 238: d by the fix tool. > I don't see how duplicate HMS tables are handled in the fix tool. If we exp Good point. I actually intended to add more suggestion hints in the check output, I'm going to do that. The reason I removed the duplicate table removing is that it's not unambiguously the right thing to do -- Todd pointed out that the admin may want to create a view with the duplicated name pointing to the Kudu table. http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@410 PS6, Line 410: " and consi > Another option to fix the orphan HMS tables (other than dropping the table) It's not possible to recreate a Kudu table from the information in the HMS, for instance primary key and partitioning are not stored in the HMS. http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@451 PS6, Line 451: og; > Why not ask user to input the desired name here? I've reconfigured this somewhat. The tool now classifies tables with Hive-incompatible into their own bucket, and the check tool outputs a suggestion for renaming each of the. Here's the output for the manual fix test: Found Kudu table(s) with Hive-incompatible names: Kudu table | Kudu table ID | Kudu master addresses --------------------------------+----------------------------------+----------------------- default.hive-incompatible-name | 85108202213a413da70f49a9eb654981 | 127.0.0.1:51579 no_database | 83d7a052e3b44a7f946446211fd39b08 | 127.0.0.1:51579 Suggestion: rename the Kudu table(s) to be Hive-compatible: $ kudu table rename_table --alter_external_catalogs=false 127.0.0.1:51579 default.hive-incompatible-name <database-name>.<table-name> $ kudu table rename_table --alter_external_catalogs=false 127.0.0.1:51579 no_database <database-name>.<table-name> I think this is more or less on par with having the tool take input, and I think it's a simpler implementation. If you feel strongly about input I can add it back, though. http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@474 PS6, Line 474: table_id > Should we use normalized table name here? Done http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@532 PS6, Line 532: it. > I think we cannot simply return early if upgrade HMS tables failed. Since t Good catch. I've inverted the control flow so that it upgrades the table in the HMS first and then renamed the kudu table next. If the rename in Kudu fails, then the next run of 'hms check' will recognize it as a table whose name doesn't match the HMS entry, and it will automatically be renamed in Kudu. I've also added a test case to cover this. http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@548 PS6, Line 548: : LOG(INFO) << "[dryrun] Upgrading legacy Impala HMS metadata for table " : << hms_table_name > I don't see any metadata change in the HMS from L551 to L568. Good catch, this was a stale comment. -- 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: 8 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 23:29:24 +0000 Gerrit-HasComments: Yes
