Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8307 )
Change subject: [webui] Add templates for tserver webui ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.h File src/kudu/tserver/tserver_path_handlers.h: http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.h@27 PS6, Line 27: template <class T> class scoped_refptr; > warning: invalid case style for class 'scoped_refptr' [readability-identifi Not my fault, tidy bot. http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@a156 PS6, Line 156: > so did we lose the ?raw ability here? I actually use this pretty often with We did, because I wasn't sure anyone used it. Since someone does, I'll keep it. http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@169 PS6, Line 169: transaction_json["trace"] = inflight_tx.trace_buffer(); > Perhaps we should be std::move()ing the large strings out of the PB into th Done http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@210 PS6, Line 210: Subst > std::to_string Done http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@220 PS6, Line 220: percentage > do you want to be using StringPrintf here to get a nice printout? it seems Done http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@225 PS6, Line 225: kArray > I'm surprised this isn't kObject.. how does this work? Yeah, that's a mistake. I think it works because EasyJson replaces the array value with an object value because I'm doing object-y things to it. http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@235 PS6, Line 235: if (replica->tablet() != nullptr) { : tablet_detail_json["target"] = Substitute("/tablet?id=$0", status.tablet_id()); : tablet_detail_json["mem_bytes"] = HumanReadableNumBytes::ToString( : replica->tablet() > isn't there a race here where tablet() can become null in between, if it's Done http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@413 PS6, Line 413: Substitute("$0", > std::to_string() here and a few other spots Done -- To view, visit http://gerrit.cloudera.org:8080/8307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c Gerrit-Change-Number: 8307 Gerrit-PatchSet: 6 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Tue, 24 Oct 2017 17:56:32 +0000 Gerrit-HasComments: Yes
