Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 )
Change subject: KUDU-2884: [hms] Improve master address matching ...................................................................... Patch Set 3: Code-Review+1 (7 comments) Minor issues. http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@3972 PS3, Line 3972: Alignment. Sidebar: I know CLion tends to do this for macros like RETURN_NOT_OK(). Need to figure why it does so. http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@3991 PS3, Line 3991: vector<string> master_addrs; : for (const auto& hp : cluster_->master_rpc_addrs()) { : master_addrs.emplace_back(hp.ToString()); : } Nit: Can use std::transform instead. http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.h@30 PS3, Line 30: <kudu/util/net/net_util.h> Nit: double quotes instead of <> brackets Another thing to teach CLion :) http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@570 PS3, Line 570: _arg Nit: _arg in the parameter name seems unnecessary. http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@571 PS3, Line 571: UnorderedHostPortSet* res) { Alignment http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@577 PS3, Line 577: for (auto hp : hp_vector) { : hp_set.emplace(std::move(hp)); : } Can simply specify vector in the constructor of the set. UnorderedHostPortSet hp_set(hp_vector.begin(), hp_vector.end()); http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@583 PS3, Line 583: vector<HostPort> addr_list; : for (const auto& addr : hp_set) { : addr_list.emplace_back(addr); : } Same here -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 08 Sep 2020 17:47:54 +0000 Gerrit-HasComments: Yes
