Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10422 )
Change subject: [webui] Convert /tablets page to mustache ...................................................................... Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/10422/1/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10422/1/src/kudu/tserver/tserver_path_handlers.cc@237 PS1, Line 237: auto make_replicas_json = [this > Might be missing something, but why does `this` need to be here? Also can t For the use of ConsensusStatePBToHtml. That function looks like it could be static, so I'll make it static and remove the this-capture. http://gerrit.cloudera.org:8080/#/c/10422/1/src/kudu/tserver/tserver_path_handlers.cc@259 PS1, Line 259: replica->tablet() != nullptr) { : replica_json["id_or_link"] = TabletLink(status.tablet_id()); : } else { : replica_json["id_or_link"] = status.tablet_id(); : } : replica_json["partition"] = : replica->tablet_metadata() : ->partition_schema() : .PartitionDebugString(replica->tablet_metadata()->partition(), : replica->tablet_metadata()->schema()); : replica_json["state"] = replica->HumanReadableState(); : if (replica->tablet() != nullptr) { : replica_json["mem_bytes"] = HumanReadableNumBytes::ToString( : replica->tablet()->mem_tracker()->consumption()); > nit: maybe pull out `replica->tablet()` and `replica->tablet_metadata()` in Done http://gerrit.cloudera.org:8080/#/c/10422/1/src/kudu/tserver/tserver_path_handlers.cc@274 PS1, Line 274: string n_bytes = ""; > Is it used at all? Done http://gerrit.cloudera.org:8080/#/c/10422/1/src/kudu/tserver/tserver_path_handlers.cc@303 PS1, Line 303: live_tablets > live_replicas? Done http://gerrit.cloudera.org:8080/#/c/10422/1/src/kudu/tserver/tserver_path_handlers.cc@307 PS1, Line 307: tombstoned_tablets > tombstoned_replicas? Done http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache File www/tablets.mustache: http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@22 PS1, Line 22: live_tablets > live_replicas? Done http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@23 PS1, Line 23: Tablets > Replicas? Live Tablet Replicas http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@71 PS1, Line 71: tablets > replicas? tablet replicas http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@71 PS1, Line 71: Tombstone > Tombstoned Done http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@72 PS1, Line 72: tablets > replicas? tablet replicas http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@80 PS1, Line 80: tablet > replica ? Done http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@82 PS1, Line 82: tablet > replica ? Done http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@96 PS1, Line 96: <th>Write buffer memory usage</th> : <th>On-disk size</th> > Are these expected for tombstones? Everything else I can see being useful, On-disk size won't be zero because of cmeta and tablet meta. Write buffer is zero, and we don't show the config because, even though it exists, it's likely an old one for the tablet and may mislead people. It is available to more sophisticated users via ksck or other CLI tools. I'll just get rid of the write buffer and config columns altogether for tombstones. http://gerrit.cloudera.org:8080/#/c/10422/1/www/tablets.mustache@118 PS1, Line 118: tablets > replicas ? Done -- To view, visit http://gerrit.cloudera.org:8080/10422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifddd74b23add52ae181b98a226b39e39df9b51ce Gerrit-Change-Number: 10422 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Thu, 17 May 2018 21:58:24 +0000 Gerrit-HasComments: Yes
