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 5: (15 comments) http://gerrit.cloudera.org:8080/#/c/17730/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17730/5//COMMIT_MSG@17 PS5, Line 17: Adjust the server startup : sequence to startup the web server and load an empty startup page right : before the filesystem is read, tablets are bootstrapped and rpc server : is started. nit: could you also mention how this affects the call ordering, i.e. when we can/should register path handlers? 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: : static bool is_initialized; Not used? 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. Maybe configure your IDE to set tabs to 2 spaces instead of 4? http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc@558 PS5, Line 558: web_server_->SetInitStatus(false); nit: we probably shouldn't need this -- instead, we should initialize it to false in WebServer's constructor http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc@560 PS5, Line 560: web_server_->set_footer_html(FooterHtml()); Let's set this before starting the webserver. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/server_base.cc@636 PS5, Line 636: web_server_->SetInitStatus(true); Would it make sense to put this down by L889, so we get the home page roughly where we would before? 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: #ifndef KUDU_SERVER_STARTUP_PATH_HANDLERS_H : #define KUDU_SERVER_STARTUP_PATH_HANDLERS_H nit: use #pragma once, and move it above the includes http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/startup_path_handler.h@26 PS5, Line 26: class MetricRegistry; : class Webserver; : : // Adds the startup page path handler : void RegisterStartupPathHandler(Webserver* webserver); : : void S nit: move these all to the left a couple spaces, given namespaces don't serve to increase indentation Or make this look a bit more like the other path handlers and wrap this in a class, and make the methods static. 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: l nit: missing a variable name. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.h@84 PS5, Line 84: : void SetInitStatus(bool); nit: add a comment explaining how callers should use this, and when. Also, maybe it would be clearer if this and related variables were called SetStartupComplete() or somesuch. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.h@210 PS5, Line 210: bool is_initialized_; This should probably be an atomic<bool>, given we expect it to be accessed from multiple threads. 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: context_(nullptr) { Initialize is_initialized_(false) here. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc@189 PS5, Line 189: /* args */ nit: remove the comment. We only use it when the argument is unused, which is no longer the case. http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc@732 PS5, Line 732: i nit: could you add a comment explaining why we need to have this check here? http://gerrit.cloudera.org:8080/#/c/17730/5/src/kudu/server/webserver.cc@831 PS5, Line 831: nit: remove spaces -- 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: 5 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: Fri, 20 Aug 2021 01:17:04 +0000 Gerrit-HasComments: Yes
