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 7: (5 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 @return sub-section. 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. 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. 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 by default might hide some places where it's necessary to properly populate the location field for an instance of KuduTabletServer::Data. 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@380 PS7, Line 380: DataTable table({ "UUID", "Address", "Status" }); Please add DCHECK_EQ(KsckServerType::MASTER, type) into the beginning of this scope to enforce consistency if KsckServerType changes. -- 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: Tue, 02 Oct 2018 04:45:42 +0000 Gerrit-HasComments: Yes
