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 13: (21 comments) http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/fs_manager.h@189 PS13, Line 189: * nit: stick the asterisk with FsReport http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/fs_manager.cc@369 PS13, Line 369: // Startup progress page "Read instance metadata files" step start nit: it's a best practice that your comments be complete sentences. Though FWIW, I don't think there will be much confusion if you removed the comments entirely. There doesn't seem to much need/room for clarification. Same elsewhere. http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/log_block_manager.cc@2516 PS13, Line 2516: containers_total nit: IMO it'd be easier to read if the variables had consistent naming (i.e. pick either total_containers or containers_total and stick with it). http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/log_block_manager.cc@2540 PS13, Line 2540: if (*containers_total == -1) { : *containers_total = static_cast<int>(containers_seen.size()); : } else { : *containers_total += static_cast<int>(containers_seen.size()); : } Seems like there's room for a TOCTOU here. Why not start at 0 and just add unconditionally? http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/server_base.cc@608 PS13, Line 608: e_metadatafiles( nit: "metadata files" is two words http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h File src/kudu/server/startup_path_handler.h: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@29 PS13, Line 29: struct StartupProgress{ nit: add a space after StartupProgress http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@30 PS13, Line 30: int percent_done; This doesn't seem very useful, given it seems to always be a surrogate for "am I done?", which can be concluded from having a valid end time. I.e. it seems like it's always either 0 or 100, unless I missed something http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@31 PS13, Line 31: MonoTime start_time; : MonoTime end_time; In general, usage of this isn't thread-safe. Consider wrapping these in setters/getters that use a spinlock to protect their values. http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@49 PS13, Line 49: StartupProgress* init() {return &init_;} : StartupProgress* read_filesystem() {return &read_filesystem_;} : StartupProgress* read_instance_metadatafiles() {return &read_instance_metadatafiles_;} : StartupProgress* read_data_directories() {return &read_data_directories_;} : StartupProgress* start_tablets() {return &start_tablets_;} : StartupProgress* initialize_master_catalog() {return &initialize_master_catalog_;} : StartupProgress* start_rpc_server() {return &start_rpc_server_;} nit: It would be clearer at call-sites that these are progress bars if these (and their private member counterparts) were suffixed with "_progress" or something. E.g. init_progress(), read_fs_progress(), etc http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@60 PS13, Line 60: bool); : void set_is_using_lbm(bool Shouldn't these have names? http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@36 PS13, Line 36: StartupProgress const& This should be "const StartupProgress&" http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@36 PS13, Line 36: s nit: capitalize this, per the GSG naming convention https://google.github.io/styleguide/cppguide.html#Function_Names http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@41 PS13, Line 41: HumanReadableElapsedTime::ToShortString : ((startup_step.end_time - startup_step.start_time).ToSeconds() nit: please follow the GSG's guidance for function calls https://google.github.io/styleguide/cppguide.html#Function_Calls http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@53 PS13, Line 53: containers_total_ = -1; : containers_processed_ = 0; nit: using magic numbers like -1 isn't a great practice. Consider using boost::optional and boost::none here http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@49 PS13, Line 49: { : tablets_total_ = -1; : tablets_processed_ = 0; : is_tablet_server_ = false; : containers_total_ = -1; : containers_processed_ = 0; : is_using_lbm_ = true; : } nit: these should be in an initializer list. http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@60 PS13, Line 60: auto * nit: pointer and reference types should have the * or & with the type. I.e. auto* http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@89 PS13, Line 89: ,st nit: add a space, same below. http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/webserver.h File src/kudu/server/webserver.h: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/webserver.h@86 PS13, Line 86: // Change the status to true once the server's initialization is completed. nit: please mention what this means for readers who are taking their first looks into the webserver. What does this mean for the webserver? http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/webserver.h@87 PS13, Line 87: SetInitStatus nit: how about calling this "SetInitialized()", which would be more consistent with the bool field. I might also consider changing them both to something like "startup complete" or somesuch, which is more aligned with the startup page http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/tserver/tablet_server.cc File src/kudu/tserver/tablet_server.cc: http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/tserver/tablet_server.cc@111 PS13, Line 111: start_time nit: why aren't we setting end_time below? I see we're setting it instead in TSTabletManager. Any reason to not have the end time set here? Also, this is already being set here, so we don't need its call in TSTabletManager. http://gerrit.cloudera.org:8080/#/c/17730/13/www/startup.mustache File www/startup.mustache: http://gerrit.cloudera.org:8080/#/c/17730/13/www/startup.mustache@32 PS13, Line 32: is_file_block_manager} nit: could we reuse is_log_block_manager for this? -- 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: 13 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: Tue, 28 Sep 2021 01:14:45 +0000 Gerrit-HasComments: Yes
