Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 )
Change subject: [location_awareness] Add location info in ksck report ...................................................................... Patch Set 13: (8 comments) > (3 comments) > > The location isn't getting piped through to the JSON output. You'll > also need to > > . Add the location to the KsckServerHealthSummaryPB in tool.proto. > . Set the location field in the proto in KsckServerHealthSummaryToPb > in ksck_results.cc > . Enhance CheckJsonVsServerHealthSummaries in ksck-test.cc so it > checks the locations set in the json vs the locations set in the > summaries. I see thanks! 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: Status > As for the order of fields, I think keeping the 'Status' before the 'Locati Makes sense, thanks! Done. http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@31 PS12, Line 31: Tablet Server Location Summary > nit: maybe, add a title for this table -- 'Tablet Server Location Summary' Done 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: rovi > nit: why this 'Json' suffix? I see there the plain (non-json) output forma Done http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1620 PS12, Line 1620: " ts-id-1 | <mock> | HEAL > nit: wrap this into NO_FATALS() ? I just noticed that other tests calling this function don't have NO_FATALS() wrapped around.? 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: > nit: substitute the location from the 'location' variable as well Done 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 > It should either by a const reference or non-const and passed-by-value with Done 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: "Status", "Location" > I added a comment on the ordering of these columns: please take a look at t Done http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc@370 PS12, Line 370: ServerHe > nit: wrap into std::move() Done -- 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: 13 Gerrit-Owner: Fengling Wang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 29 Oct 2018 23:02:36 +0000 Gerrit-HasComments: Yes
