Andrew Wong 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: (11 comments) 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@39 PS18, Line 39: const Timer& startup_step, const int percent Based on how we're using these, it seems like there are two scenarios: - 'startup_step' has enough information to determine whether we're 0 or 100 - callers explicitly pass a non-0-non-100 percent Since that's the case, how about giving a default value to percent to indicate that we should get the 0-or-100 percent from 'startup_step'? E.g. void SetWebResponse(..., percent = -1) { output->Set("...status", percent == -1 ? (startup_step->IsStopped ? 100 : 0) : percent); ... } That way we avoid any possible confusion with using the wrong timer to determine the percent. http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/server/startup_path_handler.cc@69 PS18, Line 69: read_filesystem_progress_ This should be read_instance_metadata_file_progress_, right? http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/tserver/ts_tablet_manager.cc@501 PS18, Line 501: nit: could you align these with the RETURN? http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/tserver/ts_tablet_manager.cc@1352 PS18, Line 1352: std::atomic<int>* tablets_total, nit: We aren't mutating this, so let's make the 'tablets_total' input a value (specifically an int, not an atomic<int>) instead of a pointer, and dereference at the callsites. Also reorder the arguments so in-arguments are first (tablets_total), and in-out arguments are last (tablets_processed, start_tablets). http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/tserver/ts_tablet_manager.cc@1351 PS18, Line 1351: void TSTabletManager::UpdateStartupProgress(std::atomic<int>* tablets_processed, : std::atomic<int>* tablets_total, : Timer* start_tablets) { : if (tablets_processed) { : ++*tablets_processed; : if (*tablets_processed == *tablets_total) { : start_tablets->Stop(); : } : } : } : This doesn't access any member variables, and can thus be put into an anonymous namespace. http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h File src/kudu/util/timer.h: http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h@32 PS18, Line 32: : : start_time_(MonoTime::Min()), : end_time_(MonoTime::Min()) { nit: please follow styling found in other headers with regards to initialization lists. Also see the google style guide https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h@37 PS18, Line 37: void Start() { std::lock_guard<simple_spinlock> l(lock_); start_time_ = MonoTime::Now(); } nit: please don't cram multiple statements into one line like this. It is difficult to read. http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h@38 PS18, Line 38: void Stop() { std::lock_guard<simple_spinlock> l(lock_); end_time_ = MonoTime::Now(); } Maybe also add a DCHECK that we've already called Start() http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h@40 PS18, Line 40: ; : This can be const too http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.cc File src/kudu/util/timer.cc: PS18: These seem lightweight enough (and dont have any additional include bloat) that we might as well put them in the header. http://gerrit.cloudera.org:8080/#/c/17730/18/www/startup.mustache File www/startup.mustache: http://gerrit.cloudera.org:8080/#/c/17730/18/www/startup.mustache@27 PS18, Line 27: Progress% nit: maybe "name (units)", i.e. "Progress (%)"? -- 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 20:28:38 +0000 Gerrit-HasComments: Yes
