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 1: (11 comments) Thanks! http://gerrit.cloudera.org:8080/#/c/11422/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11422/1//COMMIT_MSG@29 PS1, Line 29: Tablet Server UUID | Location : ----------------------------------+----------------------- : fc18b4bdb2ae4dd0a5717e8fe1f1de69 | <none> : 0f25d1891fce48789e06fdec493fb90e | <none> : ad868d782dc743369d96a9e958811f81 | <none> : c541b6ebc3a14f0f9bbee26d52545683 | /foo-bar0/BAAZ.9-quux : : Location | Count : -----------------------+--------- : /foo-bar0/BAAZ.9-quux | 1 : <none> | 3 > +1 Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck.h@283 PS1, Line 283: std::unordered_map<std::string, std::string> > I don't think we want to store locations like this. See my comment in ksck_ Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck.h@421 PS1, Line 421: std::string location_; > Is it possible to make this const? Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck.cc@216 PS1, Line 216: sh.ts_locations = master->ts_locations(); > The locations should come from the leader master, not from each master. The Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_remote.h File src/kudu/tools/ksck_remote.h: http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_remote.h@84 PS1, Line 84: std::shared_ptr<master::MasterServiceProxy> master_proxy_; > I don't think we need this. We should use a KuduClient to get the location Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_remote.cc@143 PS1, Line 143: string loc = server.location().empty() ? "<none>" : std::move(server.location()); > warning: std::move of the const expression has no effect; remove std::move( Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_remote.cc@467 PS1, Line 467: RemoteKsckCluster::RetrieveTabletServers > This is where we should retrieve location information from the leader maste Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_results.h@145 PS1, Line 145: std::unordered_map<std::string, std::string> > Right, we just need a single location, which will be populated only for TS Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_results.cc@367 PS1, Line 367: "\n" > here and below: prefer to use std::endl instead Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_results.cc@389 PS1, Line 389: std::unordered_map > There is 'using std::unordered_map' in the beginning, so there is no need t Done http://gerrit.cloudera.org:8080/#/c/11422/1/src/kudu/tools/ksck_results.cc@391 PS1, Line 391: for (const auto& loc : s.ts_locations) { : if (recorded_uuids.count(loc.first)) continue; : loc_table.AddRow({ loc.first, loc.second }); : recorded_uuids.insert(loc.first); : location_counts[loc.second]++; : } > I don't think this is a safe way to get up-to-date information from multipl 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: 1 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: Thu, 20 Sep 2018 22:03:02 +0000 Gerrit-HasComments: Yes
