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 1: (7 comments) 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 Is this a part of master summary as well? I thought master summary is information about masters. Maybe, separate the newly introduced information into 'Location Summary' or something like that? 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> Consider introducing a typedef for this and document what key and value contain. 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? 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> I thought KsckServerHealthSummary is just for a single server instance, why is it necessary to have map of locations in there? 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 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 to add namespace prefix here. 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 multiple masters. What if a non-leader master reports no location for a tablet server, and that information from a non-leader comes first, but the up-to-date information coming from leader master comes later? Overall, is it possible to leverage ListTServers() or from tool_action_tserver.cc to get that info from the leader master? -- 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 <fw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Wed, 12 Sep 2018 00:56:33 +0000 Gerrit-HasComments: Yes
