Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17730 )
Change subject: [WIP] KUDU-1959 - Implement server startup progress page for tablet and master servers ...................................................................... Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/17730/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17730/1//COMMIT_MSG@7 PS1, Line 7: [WIP] KUDU-1959 - Implement server startup progress page for tablet and : master servers nit: keep this on the same line, even if it's over the char limit http://gerrit.cloudera.org:8080/#/c/17730/1//COMMIT_MSG@19 PS1, Line 19: roc nit: what is this referring to? http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/server_base.cc@550 PS1, Line 550: //KUDU-1959 - nit: unless we're explicitly fixing a complicated bug, we don't usually add the jiras in code comments. http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/server_base.cc@896 PS1, Line 896: if (web_server_) { Just making sure I understand the behavior here. By the time we get here, the server is already running in a separate thread, but that's ok because the responses are generated on the fly whenever BeginRequestCallback is executed, so we'll always fetch the updated values, right? Or do we have to worry about fields being set after the webserver is started? http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.h File src/kudu/server/startup_path_handler.h: http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.h@17 PS1, Line 17: : #ifndef KUDU_SERVER_STARTUP_PATH_HANDLERS_H : #define KUDU_SERVER_STARTUP_PATH_HANDLERS_H nit: we typically use #pragma once instead of ifndef for new headers http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.h@30 PS1, Line 30: void RegisterMetricsJsonHandler(Webserver* webserver, const MetricRegistry* const metrics); This isn't defined? http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.h@23 PS1, Line 23: class MetricRegistry; : class Webserver; : : // Adds the startup page path handler : void AddStartupPathHandlers(Webserver* webserver); : : // Adds an endpoint to get metrics in JSON format. : void RegisterMetricsJsonHandler(Webserver* webserver, const MetricRegistry* const metrics); nit: move these all two spaces to the left. http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.cc@18 PS1, Line 18: #include <memory> : #include <gflags/gflags.h> nit: add a newline between these two lines to separate system headers and thirdparty headers. http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.cc@31 PS1, Line 31: : // For configuration dashboard : DECLARE_bool(webserver_require_spnego); : DECLARE_string(redact); : DECLARE_string(rpc_encryption); : DECLARE_string(rpc_authentication); : DECLARE_string(webserver_certificate_file); nit: these aren't used? or are they going to be used in a later version of this patch http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.cc@63 PS1, Line 63: static void Startup(const Webserver::WebRequest &req, Webserver::WebResponse *resp) { Not used? http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.cc@40 PS1, Line 40: namespace { : // Html/Text formatting tags : struct Tags { : string pre_tag, end_pre_tag, line_break, header, end_header; : // If as_text is true, set the html tags to a corresponding raw text representation. : explicit Tags(bool as_text) { : if (as_text) { : pre_tag = ""; : end_pre_tag = "\n"; : line_break = "\n"; : header = ""; : end_header = ""; : } else { : pre_tag = "<pre>"; : end_pre_tag = "</pre>"; : line_break = "<br/>"; : header = "<h2>"; : end_header = "</h2>"; : } : } : }; : } // anonymous namespace : : static void Startup(const Webserver::WebRequest &req, Webserver::WebResponse *resp) { : EasyJson *output = &resp->output; : } : : : void AddStartupPathHandlers(Webserver *webserver) { : bool styled = true; : bool on_nav_bar = true; : webserver->RegisterPathHandler("/Startup", "Startup", Startup, styled, on_nav_bar); : } nit: move these all a couple spaces to the left http://gerrit.cloudera.org:8080/#/c/17730/1/src/kudu/server/startup_path_handler.cc@73 PS1, Line 73: } > warning: namespace 'kudu' not terminated with a closing comment [google-rea +1 -- To view, visit http://gerrit.cloudera.org:8080/17730 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1db1fcf16261d4ced1b3657a697766a5335271b4 Gerrit-Change-Number: 17730 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 27 Jul 2021 20:52:56 +0000 Gerrit-HasComments: Yes
