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 14: (20 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: tu > nit: stick the asterisk with FsReport Done 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: std::atomic<int>* containers_total) { > nit: it's a best practice that your comments be complete sentences. Done 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 Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/fs/log_block_manager.cc@2540 PS13, Line 2540: *containers_total += containers_seen.size(); : } : : for (const string& container_name : containers_seen){ : / > Seems like there's room for a TOCTOU here. Why not start at 0 and just add Done. Re-wrote the logic to start from 0. 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: > nit: "metadata files" is two words Done 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: public: > nit: add a space after StartupProgress Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@30 PS13, Line 30: > This doesn't seem very useful, given it seems to always be a surrogate for Yes, that is not needed. Removing it http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@49 PS13, Line 49: std::atomic<int>* containers_total() {return &containers_total_;} : void set_is_tablet_server(bool is_tablet_server); : void set_is_using_lbm(bool is_using_lbm); : : private: : // Hold the initialization step progress information like the status, start and end time. : StartupProgress init_progress_; > nit: It would be clearer at call-sites that these are progress bars if thes Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.h@60 PS13, Line 60: tance metadata files progress in all the configured directories. : StartupProgress read_insta > Shouldn't these have names? Done 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: S > nit: capitalize this, per the GSG naming convention https://google.github.i Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@36 PS13, Line 36: > This should be "const StartupProgress&" Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@41 PS13, Line 41: : output->Set(step + "_time", > nit: please follow the GSG's guidance for function calls Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@53 PS13, Line 53: tablets_processed_(0), : tablets_total_(0), > nit: using magic numbers like -1 isn't a great practice. Consider using boo Changed the log and now tablets_total_ and containers_total_ are initialized to 0 and not -1 http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@49 PS13, Line 49: : } : : StartupPathHandler::StartupPathHandler(): : tablets_processed_(0), : tablets_total_(0), : containers_processed_(0), : > nit: these should be in an initializer list. Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@60 PS13, Line 60: > nit: pointer and reference types should have the * or & with the type. I.e. Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/startup_path_handler.cc@89 PS13, Line 89: > nit: add a space, same below. Done 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 webserver's startup is completed. The > nit: please mention what this means for readers who are taking their first Done http://gerrit.cloudera.org:8080/#/c/17730/13/src/kudu/server/webserver.h@87 PS13, Line 87: a kudu serve > nit: how about calling this "SetInitialized()", which would be more consist Agree. Changed it accordingly and clarified this startup completion is different from the actual server 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: >* tablets > nit: why aren't we setting end_time below? I see we're setting it instead i If I understood it right, opening the tablets is asynchronous, hence could not here but had to set it in TSTabletManager after all the tablets are opened. It makes more sense to have the start_time to be set here. Removing it from 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_log_block_manager}} > nit: could we reuse is_log_block_manager for this? 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: 14 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: Sat, 02 Oct 2021 04:30:40 +0000 Gerrit-HasComments: Yes
