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 19: (12 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: Done http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/server/startup_path_handler.cc@69 PS18, Line 69: _) { > This should be read_instance_metadata_file_progress_, right? Yes, good catch. Sorry for the typo. http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/server/startup_path_handler.cc@73 PS18, Line 73: > This should be "read_data_directories" Done 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: scop > nit: could you align these with the RETURN? Done http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/tserver/ts_tablet_manager.cc@1352 PS18, Line 1352: .ToMilliseconds(); > nit: We aren't mutating this, so let's make the 'tablets_total' input a val Done http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/tserver/ts_tablet_manager.cc@1351 PS18, Line 1351: : int elapsed_ms = (MonoTime::Now() - start).ToMilliseconds(); : if (elapsed_ms > FLAGS_tablet_start_warn_threshold_ms) { : LOG(WARNING) << LogPrefix(tablet_id) << "Tablet startup took " << elapsed_ms << "ms"; : if (Trace::CurrentTrace()) { : LOG(WARNING) << LogPrefix(tablet_id) << "Trace:" << std::endl : << Trace::CurrentTrace()->DumpToString(); : } : } : UpdateStartupProgress((tablets_total != nullptr) ? tablets_total->load() : 0, : > This doesn't access any member variables, and can thus be put into an anony Done 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 initializ Done http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h@37 PS18, Line 37: void Start() { > nit: please don't cram multiple statements into one line like this. It is d Done http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h@38 PS18, Line 38: std::lock_guard<simple_spinlock> l(lock_); > Maybe also add a DCHECK that we've already called Start() Done http://gerrit.cloudera.org:8080/#/c/17730/18/src/kudu/util/timer.h@40 PS18, Line 40: : > This can be const too Done 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) Done 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 (%)"? 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: 19 Gerrit-Owner: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 11 Oct 2021 17:26:09 +0000 Gerrit-HasComments: Yes
