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 1: (6 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? ksck output is already pretty lengthy so I don't want to introduce an extra table with #rows = #registered TS. Plus I think it's surprising to have TS locations not located with TS. So the locations should be another column in the TS table, even if it's not the easiest thing to do with ksck's data model. 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 con I don't think we want to store locations like this. See my comment in ksck_remote.cc. 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. They are a "cluster" property so a locations map belongs in KsckCluster rather than in each KsckMaster. The fact that each master is assigning a location to each TS is kind of an implementation quirk. 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 from ListTabletServers. 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@467 PS1, Line 467: RemoteKsckCluster::RetrieveTabletServers This is where we should retrieve location information from the leader master, I think. It'll be necessary to add the location to the TabletServer objects returned from ListTabletServers. Then it should be stored in the KsckTabletServer objects (can be const since we construct them here). That should make it easier to map the location information into the TS table instead of as a separate section in the master summary. 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 Right, we just need a single location, which will be populated only for TS summaries. -- 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 15:50:54 +0000 Gerrit-HasComments: Yes
