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 7: (14 comments) http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.h File src/kudu/server/server_base.h: http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.h@221 PS5, Line 221: : > Not used? Oh, no it is not. Will get rid of it. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc@557 PS5, Line 557: > nit: got a bit too many spaces. Same elsewhere. Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc@558 PS5, Line 558: AddPreInitializedDefaultPathHandlers > nit: we probably shouldn't need this -- instead, we should initialize it to Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc@560 PS5, Line 560: RETURN_NOT_OK(web_server_->Start()); > Let's set this before starting the webserver. Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc@636 PS5, Line 636: return rpc_server_->Bind(); > Would it make sense to put this down by L889, so we get the home page rough Yes, that makes sense. Good idea! http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/startup_path_handler.h File src/kudu/server/startup_path_handler.h: http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/startup_path_handler.h@21 PS5, Line 21: : namespace kudu { > nit: use #pragma once, and move it above the includes This change was made earlier when suggested, but looks like it was never saved somehow. Made it again anyway. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/startup_path_handler.h@26 PS5, Line 26: : // Adds the startup page path handler : void RegisterStartupPathHandler(Webserver* webserver); : : void Startup(const Webserver::WebRequest &req, Webserver::WebResponse *resp); : : } // nam > nit: move these all to the left a couple spaces, given namespaces don't ser Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.h File src/kudu/server/webserver.h: http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.h@85 PS5, Line 85: > nit: missing a variable name. Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.h@84 PS5, Line 84: bool IsSecure() const; : > nit: add a comment explaining how callers should use this, and when. Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.h@210 PS5, Line 210: struct sq_context* co > This should probably be an atomic<bool>, given we expect it to be accessed Done. Added a comment too. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc@179 PS5, Line 179: s_initialized_(false > Initialize is_initialized_(false) here. Done. Thanks http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc@189 PS5, Line 189: > nit: remove the comment. We only use it when the argument is unused, which Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc@732 PS5, Line 732: } > nit: could you add a comment explaining why we need to have this check here Done http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc@831 PS5, Line 831: > nit: remove spaces Done -- 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: 7 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 31 Aug 2021 21:07:06 +0000 Gerrit-HasComments: Yes
