Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11313 )

Change subject: [location_awareness] Add 'location' column in tserver list
......................................................................


Patch Set 5:

(4 comments)

overall looks good, just a few nits

http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2038
PS5, Line 2038: Listi
Is this a typo?  'List' or 'Listing'?


http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2043
PS5, Line 2043: "--location_mapping_cmd=" +
              :         Substitute("$0 $1", kLocationCmdPath, location)
nit: why not just

Substitute("--location_mapping_cmd=$0 $1", kLocationCmdPath, location)

?


http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2057
PS5, Line 2057: Listi
ditto: is this a typo?


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

http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/tool_action_tserver.cc@144
PS5, Line 144: loc
nit: add std::move:

std::move(loc)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 5
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Fri, 31 Aug 2018 05:48:18 +0000
Gerrit-HasComments: Yes

Reply via email to