Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8307 )
Change subject: [webui] Add templates for tserver webui ...................................................................... Patch Set 6: (7 comments) 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 'curl' to get output in a readable format over an ssh connection. Not sure if JSON would be as readable. 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 the JSON to avoid copy? http://gerrit.cloudera.org:8080/#/c/8307/6/src/kudu/tserver/tserver_path_handlers.cc@210 PS6, Line 210: Subst std::to_string 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 like in the template you're just including this directly rather than something like %%%.1f or whatever 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? 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 shutting down? 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 (though I thought easyjson could hold ints too) -- 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 <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 23 Oct 2017 23:53:13 +0000 Gerrit-HasComments: Yes
