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 3: (5 comments) I agree with the direction of this approach, starting the webserver earlier, and only registering path handlers as they become ready. I wonder if we could have the default page be the /startup page when no other pages are available. http://gerrit.cloudera.org:8080/#/c/17730/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17730/3/src/kudu/server/server_base.cc@552 PS3, Line 552: AddStartupPathHandlers(web_server_.get()); : RETURN_NOT_OK(web_server_->Start()); nit: minor rewording, and it's worth adding a note about why this is done separately from the other handlers. // Register the startup web handler and start the web server to make the web UI // available while the server is initializing, loading the file system, etc. // // NOTE: unlike the other path handlers, we register this path handler // separately, as the startup handler is meant to be displayed before all of // Kudu's subsystems have finished initializing. http://gerrit.cloudera.org:8080/#/c/17730/3/src/kudu/server/server_base.cc@850 PS3, Line 850: //KUDU-1959 - Load the footer with UUID only if the UUID is available nit: move this comment to inside the function, since it describes a portion of the implementation, rather than the entire function itself. 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@30 PS1, Line 30: // void RegisterMetricsJsonHandler(Webserver* webserver, const MetricRegistry* const metrics > I can actually comment it out for now, it will be needed in future though We try to make sure we keep merged code as clean as possible, even if we're working on a series of patches. If this method won't be used in this patch, let's just remove it and add it in when it is actually used. If you intend on using it before this patch is merged (i.e. in a later iteration of this patch), it's fine to keep it as is, as long as your reviewers know it will be used later on. http://gerrit.cloudera.org:8080/#/c/17730/3/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17730/3/src/kudu/server/startup_path_handler.cc@69 PS3, Line 69: AddStartupPathHandlers nit: to be consistent with existing terminology, prefer calling this RegisterStartupPathHandler (singular) or somesuch http://gerrit.cloudera.org:8080/#/c/17730/3/src/kudu/server/startup_path_handler.cc@72 PS3, Line 72: Startup nit: should be lowercased? -- 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: 3 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, 13 Aug 2021 01:30:22 +0000 Gerrit-HasComments: Yes
