Bankim Bhavsar has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17141 )

Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster 
ksck
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@657
PS5, Line 657:   if (master.empty())
             :       continue;
Nit: I'm not sure whether it's part of style guide but for "if" if there is a 
single stmt and without any braces then the stmt should be on the same single 
line.
if (master.empty()) continue;


http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@660
PS5, Line 660: er)+"
> That said, maybe instead keep an unordered_set of duplicated masters and 
> output that via JoinStrings()? (see gutil/strings/join.h)

+1 to above.


http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@665
PS5, Line 665:   if (!duplicate_masters.empty())
             :     return Status::InvalidArgument("Duplicate master addresses 
specified: " + duplicate_masters);
             :   *master_addresses = std::move(master_addresses_local);
Same as above about either using braces {} for "if" if the stmt is on next line.
Else it can cause confusion whether the L667 is part of the if stmt or no.



--
To view, visit http://gerrit.cloudera.org:8080/17141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f
Gerrit-Change-Number: 17141
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <achenn...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abu...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com>
Gerrit-Reviewer: Grant Henke <granthe...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 10 Mar 2021 21:54:54 +0000
Gerrit-HasComments: Yes

Reply via email to