Abhishek Chennaka 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: (7 comments) http://gerrit.cloudera.org:8080/#/c/17730/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17730/1//COMMIT_MSG@19 PS1, Line 19: roc > nit: what is this referring to? rpc* Sorry for the Typo 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 Right, its more for a better searchability while I continue working on this. Was planning to remove at the end of implementation. 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, t >From what I tested, even after the web server is started above, I could see >that the other pages were loaded once this part of the code is executed. 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 Okay, Will fix it 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? I can actually comment it out for now, it will be needed in future though 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@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 Right, they'd be used in the alter version of the 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? Will be used from next version onwards, its more of a WIP thing -- 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: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 28 Jul 2021 02:58:25 +0000 Gerrit-HasComments: Yes
