Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17730 )
Change subject: KUDU-1959 - Implement server startup progress page for tablet and master servers ...................................................................... Patch Set 18: (21 comments) http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.h File src/kudu/server/startup_path_handler.h: http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.h@79 PS16, Line 79: std > Shouldn't this be atomic too? Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc@37 PS16, Line 37: > nit: since this is meant to be only call const methods, it should be marked Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc@41 PS16, Line 41: tput->Set(step + "_time", HumanReadableElapsedTime::ToShortString( : (startup_step.TimeElapsed()).ToSeconds())); : } : : StartupPathHandler::StartupPathHandler(): : tablets_processed_(0), : tablets_total_(0), : containers_processed_(0), : c > If switching to the Timer::{Start,Stop} API, consider also adding an Elapse Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc@72 PS16, Line 72: m_) { > nit: if going the route of making this a more generic Timer class, consider Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc@100 PS16, Line 100: SetWebResponse(output, "start_rpc_server", start_rpc_server_progress_, : (start_rpc_server_progress_.IsStopped()) ? 100 : 0); : } : : void StartupPathHandler::RegisterStartupPathHandler(Webserver *webserver) { : bool styled = true; : bool on_nav_bar = true; : webserver->RegisterPathHandler("/startup", "Startup", : [this](const Webserver::WebRequest& req, : > nit: don't we have a method for this? Consider simplifying this logic to so Done http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/server/startup_path_handler.cc@73 PS18, Line 73: read_data_directories_progress This should be "read_data_directories" http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/webserver.cc@195 PS16, Line 195: & > nit: move the ampersand to the left Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/webserver.cc@734 PS16, Line 734: // As the home page is redirected to startup until the server's initialization is comple > nit: the what is clear from this code; it'd be great if you could explain t Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.h@330 PS16, Line 330: // > nit: remove this extra comment line Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@390 PS16, Line 390: std > This also needs to be atomic, in order to ensure reads and writes happen at Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@594 PS16, Line 594: // W > nit: could you move this four spaces to the left? Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@1243 PS16, Line 1243: const > The value is actually being mutated, so it shouldn't be const. Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@1349 PS16, Line 1349: > nit: fix spacing Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@1353 PS16, Line 1353: Timer* start_tablets) { : if (tablets_processed) { : + > Why do this here instead of in tablet_server.cc? In general the start and e This part of opening the tablets is asynchronous hence had to set the end time here and not in tablet_server.cc. We can get the start time to be set in ts_tablet_manager.cc for keeping it consistent. http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h File src/kudu/util/startup_progress.h: http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@19 PS16, Line 19: > nit: add a newline after <atomic> to separate the system headers from our h Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@20 PS16, Line 20: > nit: we can remove this; it doesn't appear to be used Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@25 PS16, Line 25: > nit: we typically only use the kudu namespace for things in the util direct Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@28 PS16, Line 28: > Now that we've boiled this down to its basics, it looks very much like a pr Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@32 PS16, Line 32: > nit: this should return a MonoTime, not a const Monotime*. The difference i Done http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.cc File src/kudu/util/startup_progress.cc: http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.cc@26 PS16, Line 26: : : : : : : : : : : : : : > nit: IMO this is simple enough that we might as well put this all in the he Done http://gerrit.cloudera.org:8080/#/c/17730/16/www/startup.mustache File www/startup.mustache: PS16: > nit: while we don't have a strict style guide for HTML, it's probably worth 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: 18 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: Thu, 07 Oct 2021 17:55:19 +0000 Gerrit-HasComments: Yes
