Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 )
Change subject: [location_awareness] Add location info in ksck report ...................................................................... Patch Set 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@24 PS12, Line 24: Location As for the order of fields, I think keeping the 'Status' before the 'Location' (if counting from the left to right) would make more sense: 1) It's more 'compatible' with the former output (no need to update scripts that are parsing the output, if any exist). 2) The status of a tablet server feels more important field that people would look at usually. 2) Locations most likely to be multi-component hierarchical long strings: it's better to keep that sort of fields in the end of every row. http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@31 PS12, Line 31: Location | Count nit: maybe, add a title for this table -- 'Tablet Server Location Summary' looks like a good one to me. http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1601 PS12, Line 1601: Json nit: why this 'Json' suffix? I see there the plain (non-json) output format is also being verified. http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1620 PS12, Line 1620: CheckJsonStringVsKsckResults nit: wrap this into NO_FATALS() ? http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote-test.cc@540 PS12, Line 540: /foo nit: substitute the location from the 'location' variable as well http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h File src/kudu/tools/ksck_remote.h: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h@95 PS12, Line 95: const nit: what does it buy having const for the HostPort parameter passed by value here? http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc@365 PS12, Line 365: "Location", "Status" I added a comment on the ordering of these columns: please take a look at the commit message. http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc@370 PS12, Line 370: location nit: wrap into std::move() -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624debbbb2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 12 Gerrit-Owner: Fengling Wang <fw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 29 Oct 2018 17:14:19 +0000 Gerrit-HasComments: Yes
