Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13490 )
Change subject: [hms] Add a tool to list all Kudu HMS entries ...................................................................... Patch Set 2: (6 comments) LGTM, just a few nits http://gerrit.cloudera.org:8080/#/c/13490/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: PS2: It would be great to add coverage for newly introduced commands into: * TEST_F(ToolTest, TestTopLevelHelp) * TEST_F(ToolTest, TestModeHelp) http://gerrit.cloudera.org:8080/#/c/13490/2/src/kudu/tools/kudu-tool-test.cc@3887 PS2, Line 3887: // Enable the HMS integration. : cluster_->ShutdownNodes(cluster::ClusterNodes::MASTERS_ONLY); : cluster_->EnableMetastoreIntegration(); What does the tool output in case if the integration disabled? Does it make sense to add coverage for that case? http://gerrit.cloudera.org:8080/#/c/13490/2/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/13490/2/src/kudu/tools/tool_action_hms.cc@794 PS2, Line 794: RETURN_NOT_OK(PrintHMSTables(hms_tables, cout)); : : return Status::OK(); nit: maybe just return PrintHMSTables(hms_tables, cout); ? http://gerrit.cloudera.org:8080/#/c/13490/2/src/kudu/tools/tool_action_hms.cc@857 PS2, Line 857: string( nit: I would expect this extra object was not needed, since Substitute() returns std::string already. http://gerrit.cloudera.org:8080/#/c/13490/2/src/kudu/tools/tool_action_hms.cc@859 PS2, Line 859: string( nit: ditto http://gerrit.cloudera.org:8080/#/c/13490/2/src/kudu/tools/tool_action_hms.cc@870 PS2, Line 870: .AddAction(std::move(hms_check)) : .AddAction(std::move(hms_downgrade)) : .AddAction(std::move(hms_fix)) : .AddAction(std::move(hms_precheck)) : .AddAction(std::move(hms_list)) nit: sort alphabetically; same for the Action within 815-868 line span -- To view, visit http://gerrit.cloudera.org:8080/13490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib18cde6e92e799390bb977562fd8d2eac2dae026 Gerrit-Change-Number: 13490 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 01 Jun 2019 06:25:27 +0000 Gerrit-HasComments: Yes
