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 20: Code-Review+1 (9 comments) Mostly just nits about the comments, but this is looking pretty much good to go IMO otherwise! http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/fs/block_manager.h@223 PS20, Line 223: // If the containers files in the data directories are being processed, we : // provide the total container files count and processed countainer count : // until that instant to server startup page nit: In general, when writing comments, we try to keep the details caller-agnostic. If a caller of this function didn't want to supply them to a startup page, that's fine. Instead, just mention what the returned values are, rather than how callers should use them. http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/fs/fs_manager.h@187 PS20, Line 187: // In addition the data for the startup progress web page will also be : // populated here and in the subsequent calls made here. nit: same feedback here http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/fs/log_block_manager.h@371 PS20, Line 371: // The total containers present and the opened/processed needed for the : // startup page progress are populated here. nit: same feedback here http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/server/startup_path_handler.cc@83 PS20, Line 83: // Populate the processed containers and total containers in case of log block manager nit: this comment seems misplaced, or perhaps a duplicate of L69? http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/server/startup_path_handler.cc@107 PS20, Line 107: [this](const Webserver::WebRequest& req, : Webserver::WebResponse* resp) { : this->Startup(req, resp);}, nit: please follow the GSG's guidance for multi-line lambdas https://google.github.io/styleguide/cppguide.html#Formatting_Lambda_Expressions http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/tserver/ts_tablet_manager.h@330 PS20, Line 330: // It also updates the startup progress page by updating the tablets processed, : // total tablets and the timestamp of when all the tablets are processed. nit: same feedback here. It's probably also worth mentioning that these may get set after returning from this method, since bootstrapping is asynchronous. http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/tserver/ts_tablet_manager.h@332 PS20, Line 332: : nit: remove the extra newline http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/tserver/ts_tablet_manager.h@393 PS20, Line 393: : // Update the startup page with the recent startup progress nit: same feedback here; there's nothing inherently related to startup pages here now, only atomic ints and timers. How about "Increments the number of tablets processed, and if that number has reached 'tablets_total', stops the 'start_tablets' timer. If 'tablets_processed' is a nullptr (e.g. in some tests), this is a no-op." Also, now that we're no longer passing a startup progress page here, let's be more descriptive and call this IncrementTabletsProcessed() or something so it's even clearer what this is doing at callsites. http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/util/timer.h File src/kudu/util/timer.h: http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/util/timer.h@43 PS20, Line 43: DCHECK(start_time_ != MonoTime::Min()); nit: this DCHECK needs to be under the lock as well. Also consider using DCHECK_NE? -- 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: 20 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: Mon, 11 Oct 2021 18:51:30 +0000 Gerrit-HasComments: Yes
