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 22: (10 comments) Thanks for the review Alexey 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 vi As discussed, we need a startup page needs a handle on these value to update them as and when needed. We can use the metrics which are implemented in later patches as an alternative way of achieving the goal in a cleaner manner. Will raise a followup patch for that. We can do the same approach for tablets processed/total count too probably. 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 par Since the declaration contains these parameters, included them here. If we ever go on a route of opening the file block containers during startup we can use these I guess. But the cleaner approach would be the one discussed here https://gerrit.cloudera.org/#/c/17730/22/src/kudu/fs/block_manager.h@227 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 Done 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 F Discussed this below https://gerrit.cloudera.org/#/c/17730/22/src/kudu/fs/block_manager.h@227 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? Done 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 Done and tested too. Works. 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 t Agree, moved this up and now it doesn't render the home page is the startup is not complete. 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 t Yes, there was a discussion about it but we thought stopwatch might provide more than needed functionality for this purpose i.e. we just need total time between two points and hence implemented as a simpler version of stopwatch 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? Done 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, w 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: 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 21:03:26 +0000 Gerrit-HasComments: Yes
