Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@9 PS6, Line 9: a > nit: an Done http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@13 PS6, Line 13: > nit: expected Done http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py File build-support/iwyu.py: http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py@86 PS6, Line 86: "src/kudu/server/webserver.cc", : "src/kudu/util/bit-util-test.cc", > nit: just in case if you haven't looked at that yet, Todd added a dependenc Done http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@92 PS6, Line 92: table_name > What if 'table_name' is already a 'new' one? I.e., my concern is that this I think it is hard to determine whether the tables are legacy or not by names. Since legacy tables can also contain '.' in the name. I think it is safe to assume the admin run this tool as the first thing to do when enabling hms integration. http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@142 PS6, Line 142: URIs > nit: URIs Done http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@188 PS6, Line 188: .AddRequiredParameter({ kDefaultDatabaseArg, kDefaultDatabaseArgDesc }) : .AddOptionalParameter("hive_metastore_uris") : .AddOptionalParameter("hive_metastore_sasl_enabled") : .AddOptionalParameter("hive_metastore_retry_count") : .AddOptionalParameter("hive_metastore_send_timeout") : .AddOptionalParameter("hive_metastore_recv_timeout") > usability nit: does it make sense to remove the 'hive_metastore_' prefix ? All of these flags are already defined in hms_catalog.cc. Here just reuse these flags to avoid double flags. -- 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: 7 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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, 26 Apr 2018 05:18:35 +0000 Gerrit-HasComments: Yes
