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 21: (9 comments) 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 'containers_processed' and 'containers_total' are not nullptr, they will : // be populated with total containers attempted to be opened/processed and : // total containers present respectively. > nit: In general, when writing comments, we try to keep the details caller-a Done 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: // If 'read_instance_metadata_files' and 'read_data_directories' are not nullptr, : // they will be populated with time spend reading the in > nit: same feedback here Done 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: // If 'containers_processed' and 'containers_total' are not nullptr, they will : // be populated with total containers attemp > nit: same feedback here Done 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: // Set the bootstrapping and opening tablets step and handle the case of zero tablets > nit: this comment seems misplaced, or perhaps a duplicate of L69? Yes, that should not be there. Removed now http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/server/startup_path_handler.cc@107 PS20, Line 107: this->Startup(req, resp); : }, : styled, on_nav_bar); > nit: please follow the GSG's guidance for multi-line lambdas Done 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: // opening the tablet is complete (in either a success or a failure case). : // > nit: same feedback here. It's probably also worth mentioning that these may Done http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/tserver/ts_tablet_manager.h@332 PS20, Line 332: // In the subsequent call made to UpdateStartupProgress, 'tablets_processed' : > nit: remove the extra newline Done http://gerrit.cloudera.org:8080/#/c/17730/20/src/kudu/tserver/ts_tablet_manager.h@393 PS20, Line 393: void SetNextUpdateTimeForTests(); : > nit: same feedback here; there's nothing inherently related to startup page Done 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 DC For DCHECK_NE, appears we need to implement the operator operator<<(ostream) for MonoTime. Will do that as a separate patch given this patch is bulky already. -- 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: 21 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, 18 Oct 2021 21:48:56 +0000 Gerrit-HasComments: Yes
