Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 6: (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 http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@13 PS6, Line 13: expect nit: expected 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/tools/tool_action_hms.cc", : "src/kudu/tools/kudu-tool-test.cc", nit: just in case if you haven't looked at that yet, Todd added a dependency of the IWYU-related targets on related Thrift auto-generated stuff, so this might be not necessary anymore. 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 methods does not look re-enterable, but I might be missing something. As I understand, kudu_client->ListTables() would output not only legacy tables, right? http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@142 PS6, Line 142: Uris nit: URIs http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@188 PS6, Line 188: .AddOptionalParameter("hive_metastore_uris") : .AddOptionalParameter("hive_metastore_sasl_enabled") : .AddOptionalParameter("hive_metastore_retry_count") : .AddOptionalParameter("hive_metastore_send_timeout") : .AddOptionalParameter("hive_metastore_recv_timeout") : .AddOptionalParameter("hive_metastore_conn_timeout") usability nit: does it make sense to remove the 'hive_metastore_' prefix ? If the sub-command is 'hms', doesn't it imply that all those options are about Hive metastore? If not, then maybe at least replace 'hive_metastore_' with 'hms_'? -- 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: 6 Gerrit-Owner: Hao Hao <hao....@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: Tue, 24 Apr 2018 20:57:44 +0000 Gerrit-HasComments: Yes
