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 8: (11 comments) http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/client.h@603 PS7, Line 603: > styling nit: add two extra spaces, the sentence is the continuation of the Done http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/client.h@604 PS7, Line 604: const KUDU_NO_EXPORT std::string& lo > +1 for KUDU_NO_EXPORT -- that's better than the friendship between classes. Done http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/client.cc@574 PS7, Line 574: KuduTabletServer::Data(ts_info.permanent_uuid(), hp, > Missing location. Done http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/client.cc@1588 PS7, Line 1588: client_server->data_ = new KuduTabletServer::Data(rts->permanent_uuid(), : host_ports[0], > Missing location. Done http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/tablet_server-internal.h File src/kudu/client/tablet_server-internal.h: http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/client/tablet_server-internal.h@31 PS7, Line 31: ); > Why to make this parameter by default? I think that having this parameter Done 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: EST_F(RemoteKsckTest, TestClusterWithLocation) { : // There is no location assigned for the existing three tablet servers. : // With the flag set, the newly added server will be assiged with loc > nit: These comments are good but feel out of place before the test. I think Done http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_remote-test.cc@433 PS7, Line 433: md = strings::Substitut > nit: It's kinda funky to use this string as the location string. I know it Done 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, : " <none> | 3\n"); : ASSERT_STR_CONTAINS(err_string, : " /foo | 1\n"); : } > Can this be one check on the structure of the whole table instead of 3 sepa As I tested, the order of the rows changed every now and then. So maybe checking separately is safer? 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 to Done 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: RETURN_NOT_OK(table.PrintTo(out)); : > Seems reasonable. Done http://gerrit.cloudera.org:8080/#/c/11422/7/src/kudu/tools/ksck_results.cc@380 PS7, Line 380: RETURN_NOT_OK(loc_stats_table.PrintTo(out)); > Please add DCHECK_EQ(KsckServerType::MASTER, type) into the beginning of th 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: 8 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 Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 05 Oct 2018 21:45:30 +0000 Gerrit-HasComments: Yes
