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

Reply via email to