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 16:

(20 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: int
Shouldn't this be atomic too?


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: StartupProgress&
nit: since this is meant to be only call const methods, it should be marked 
const.


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc@41
PS16, Line 41: else if (percent == 100) {
             :     output->Set(step + "_time",
             :         HumanReadableElapsedTime::ToShortString(
             :             (*startup_step.end_time() - 
*startup_step.start_time()).ToSeconds()));
             :   } else {
             :     output->Set(step + "_time",
             :         HumanReadableElapsedTime::ToShortString(
             :             (MonoTime::Now() - 
*startup_step.start_time()).ToSeconds()));
             :   }
If switching to the Timer::{Start,Stop} API, consider also adding an 
ElapsedTime call that uses MonoTime::Now() if the stop time isn't initialized. 
Then, there'd be no need for the "else if"


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc@72
PS16, Line 72: (*init_progress_.end_time() == MonoTime::Min())
nit: if going the route of making this a more generic Timer class, consider 
adding a IsStopped() method that does this comparison


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/startup_path_handler.cc@100
PS16, Line 100:   if (tablets_total_ == 0 && 
*start_tablets_progress_.end_time() == MonoTime::Min()) {
              :     output->Set("start_tablets_status", 0);
              :     output->Set("start_tablets_time", 
HumanReadableElapsedTime::ToShortString(0));
              :   } else if (tablets_total_ == 0 && 
*start_tablets_progress_.end_time() != MonoTime::Min()) {
              :     output->Set("start_tablets_status", 100);
              :     output->Set("start_tablets_time", 
HumanReadableElapsedTime::ToShortString(0));
              :   } else {
              :     percent = tablets_processed_ * 100 / tablets_total_;
              :     SetWebResponse(output, "start_tablets", 
start_tablets_progress_, percent);
              :   }
nit: don't we have a method for this? Consider simplifying this logic to 
something like

SetWebResponse(output, "start_tablets", timer_,
    tablets_total_ == 0
        ? (timer_->IsStopped() ? 100 : 0)
        : tablets_processed * 100 / tablets_total_);

Similar adjustments can maybe be made above with "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


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/server/webserver.cc@734
PS16, Line 734:   // Do not use the template for home page if the server's 
initialization is not complete.
nit: the what is clear from this code; it'd be great if you could explain the 
_why_, since at first glance this is a bit surprising.


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


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: int
This also needs to be atomic, in order to ensure reads and writes happen 
atomically (no intermediate garbage value is read when concurrently written)


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@594
PS16, Line 594:
nit: could you move this four spaces to the left?


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.


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@1349
PS16, Line 1349:
nit: fix spacing


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/tserver/ts_tablet_manager.cc@1353
PS16, Line 1353:     if (*tablets_processed == *tablets_total) {
               :       start_tablets->SetEndTime(MonoTime::Now());
               :     }
Why do this here instead of in tablet_server.cc? In general the start and end 
calls seem to always happen in pairs, so it seems odd to split them up like 
this. Any particular reason for doing so?


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: #include <atomic>
nit: add a newline after <atomic> to separate the system headers from our 
headers


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@20
PS16, Line 20: #include "kudu/server/webserver.h"
nit: we can remove this; it doesn't appear to be used


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@25
PS16, Line 25: namespace server {
nit: we typically only use the kudu namespace for things in the util directory.


http://gerrit.cloudera.org:8080/#/c/17730/16/src/kudu/util/startup_progress.h@28
PS16, Line 28: StartupProgress
Now that we've boiled this down to its basics, it looks very much like a pretty 
simple timer. How about leaning into that and calling this Timer, and making 
its API mutable Start() and Stop() that just set the start and end times to 
MonoTime::Now()? That way all the call-sites don't have to call MonoTime::Now().

In general, it's a good practice to make the API as simple and generic as we 
can, and in this case there doesn't seem to be anything specific to startup in 
this class.


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 is 
that returning a MonoTime returns a copy of start_time_, which _should_ be 
copied while lock_ is held.

Once you do that, the method should be marked const too, since the method 
doesn't mutate state. I.e.

class StartupProgress {
 public:
  MonoTime start_time() const {
    std::lock_guard<simple_spinlock> l(lock_);
    return start_time_;
  }
  ...
  mutable simple_spinlock lock_;
};

Same below.


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: StartupProgress::StartupProgress() :
             :     start_time_(MonoTime::Min()),
             :     end_time_(MonoTime::Min()) {
             : }
             :
             : void StartupProgress::SetEndTime(const MonoTime &end_time) {
             :   std::lock_guard<simple_spinlock> l(lock_);
             :   StartupProgress::end_time_ = end_time;
             : }
             :
             : void StartupProgress::SetStartTime(const MonoTime &start_time) {
             :   std::lock_guard<simple_spinlock> l(lock_);
             :   StartupProgress::start_time_ = start_time;
             : }
nit: IMO this is simple enough that we might as well put this all in the header


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 
keeping the spacing styling consistent with what we have elsewhere e.g.
https://github.com/apache/kudu/blob/master/www/masters.mustache



--
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: 16
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: Tue, 05 Oct 2021 08:18:24 +0000
Gerrit-HasComments: Yes

Reply via email to