Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 )
Change subject: [location_awareness] Add location info in ksck report ...................................................................... Patch Set 7: (6 comments) Looks good, but needs support for printing location in JSON. http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck.h@312 PS7, Line 312: : uuid_(std::move(uuid)), location_(std::move(location)) nit: Usually, we place each initializer on its own line: : uuid(std::move(uuid)), location(std::move(location)) {} http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_remote-test.cc@427 PS7, Line 427: / Test that the ksck tablet server health summary should contain location info. : // The newly added tablet server should have the assigned location displayed. : // The existing tablet servers should have location '<none>' displayed. nit: These comments are good but feel out of place before the test. I think the name of the test is good enough to explain basically what it's about. The first sentence can be removed, the second can go before L448, the third can go before L455, and you should add a comment before the L431-436 section explaining that, initially, there is no assignment for TS locations, so the TS will register with no location, but after the flag is set, the new server that registers will be assign the location '/foo-bar0/BAAZ.9-quux'. http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_remote-test.cc@433 PS7, Line 433: "/foo-bar0/BAAZ.9-quux" nit: It's kinda funky to use this string as the location string. I know it comes from one of my tests but that's because I was testing all valid characters. I think just 'foo' suffices. http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_remote-test.cc@460 PS7, Line 460: ASSERT_STR_CONTAINS(err_string, : " Location | Count\n" : "-----------------------+---------\n"); : : ASSERT_STR_CONTAINS(err_string, : " /foo-bar0/BAAZ.9-quux | 1\n"); : : ASSERT_STR_CONTAINS(err_string, : " <none> | 3\n"); Can this be one check on the structure of the whole table instead of 3 separate ones? http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_remote-test.cc@469 PS7, Line 469: } We also need to verify that the location ends up in the json output. See tool.proto, KsckResults::PrintJsonTo, KsckResults::ToPb, and CheckJsonVsKsckResults in ksck-test.cc. It suffices just to set a location on a MockKsckTabletServer in ksck-test and verify it ends up in the JSON...no need to use the RemoteKsckTabletServer for that. Make sure you test how things look both when the location is set and when it isn't. I think if location isn't set then the JSON element for location should be absent, rather than being set to an empty string. http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_results.cc@372 PS7, Line 372: // Print location count table. : out << std::endl; Corner case: will this print out an awkward extra line break if there's no tablet servers? I assume the tables will not print if there's no rows? -- 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: 7 Gerrit-Owner: Fengling Wang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 01 Oct 2018 22:17:08 +0000 Gerrit-HasComments: Yes
