Alexey Serbin 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 22: (10 comments) Took a quick look: mostly nits, overall look pretty good. http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/block_manager.h@226 PS22, Line 226: std::atomic<int>* containers_processed = nullptr, : std::atomic<int>* containers_total = nullptr) = 0 Since these are cumulative counters, would it be cleaner to expose these via dedicated accessors or a method which returns some structure with metrics? What do you think? http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/file_block_manager.h File src/kudu/fs/file_block_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/file_block_manager.h@83 PS22, Line 83: std::atomic<int>* containers_processed = nullptr, : std::atomic<int>* containers_total = nullptr Passing atomics like this looks is a bit strange to me -- are these new parameters used in the implementation at all? From file_block_manager.cc, I could not see that the code uses these at all, but I might be missing something. http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/fs_manager.h@188 PS22, Line 188: spend nit: spent http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/fs_manager.h@191 PS22, Line 191: // 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 in the subsequent calls made to : // the block manager. If those are cumulative counters, why not to expose them via accessors to FsManager (and return simple int counters, not atomics), instead of having them as out parameter for this Open() method? http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/fs/log_block_manager.cc@2515 PS22, Line 2515: Status* result_status, std::atomic<int>* containers_processed, : std::atomic<int>* containers_total) { style nit: why not to keep one parameter per line, as in the original code? http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/server/server_base.cc@498 PS22, Line 498: () nit: you omit drop the parentheses http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/server/webserver.cc@734 PS22, Line 734: // As the home page is redirected to startup until the server's initialization is complete, : // do not use the template for home page if the server's initialization is not complete. : if (!is_started_ && (path == "/home")) { : return false; : } I saw you addressed Andrew's comment from PS16, but I'm not sure this is the best place to check for that condition. Maybe, a better place for this check is somewhere in Webserver::RegisterPathHandler()? http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/util/timer.h File src/kudu/util/timer.h: PS22: I guess I missed some discussion in prior versions, but why to introduce this extra class when there is already Stopwatch in util/stopwatch.h? With Stopwatch it's possible to report on wall, user and system times. http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/util/timer.h@57 PS22, Line 57: std::lock_guard<simple_spinlock> l(lock_); : return (end_time_ != MonoTime::Min()); What if the caller called Start() second time? Maybe, add DCHECK() into Start() to prevent re-using the instance of Timer? http://gerrit.cloudera.org:8080/#/c/17730/22/src/kudu/util/timer.h@62 PS22, Line 62: MonoTime start_time_; : MonoTime end_time_; : mutable simple_spinlock lock_; Since atomics are already used in src/kudu/server/startup_path_handler.h, would it make sense to use std::atomic<MonoTime> for start_time_ and end_time_ here as well? MonoTime is eligible for std::atomic since it's just int64_t under the hood. -- 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: 22 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Wed, 20 Oct 2021 00:54:38 +0000 Gerrit-HasComments: Yes
