Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17903 )
Change subject: KUDU-1959 - Implement aggregate startup progress metrics ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/17903/5//COMMIT_MSG Commit Message: PS5: It's probably worth adding a small test for this, or maybe reusing some of your existing tests from your other patches to check these metrics http://gerrit.cloudera.org:8080/#/c/17903/5//COMMIT_MSG@9 PS5, Line 9: We expose the below metrics as a part of this commit: : * startup_progress_steps_remaining : count of server startup steps which : are yet to be completed. This value is in the range [0,4]. : * startup_progress_time_elapsed : the time elapsed so far for the server to : startup. If the startup is completed, this is the total time taken for the : startup. This is in milliseconds. : These metrics are primarily expected to be used by third party monitoring tools : to see how long has the server taken to startup historically for any sort of : trend analysis. : The startup_progress_time_elapsed metric can also be used to check the : previous startup time as an alternative to the startup page in the WebUI. style nit: it's personal preference, but it'd be nice if this weren't so condensed, and more easily parsable without having to mentally separate bullets and paragraphs. E.g. We expose the below metrics as a part of this commit: * startup_progress_steps_remaining : count of server startup steps which are yet to be completed. This value is in the range [0,4]. * startup_progress_time_elapsed : the time elapsed so far for the server to startup. If the startup is completed, this is the total time taken for the startup. This is in milliseconds. These metrics are primarily expected to be used by third party monitoring tools to see how long has the server taken to startup historically for any sort of trend analysis. The startup_progress_time_elapsed metric can also be used to check the previous startup time as an alternative to the startup page in the WebUI. http://gerrit.cloudera.org:8080/#/c/17903/5/src/kudu/server/startup_path_handler.h File src/kudu/server/startup_path_handler.h: http://gerrit.cloudera.org:8080/#/c/17903/5/src/kudu/server/startup_path_handler.h@57 PS5, Line 57: int32 nit: just use an int https://google.github.io/styleguide/cppguide.html#Integer_Types http://gerrit.cloudera.org:8080/#/c/17903/5/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17903/5/src/kudu/server/startup_path_handler.cc@143 PS5, Line 143: int counter = (init_progress_.IsStopped() ? 0 : 1) : + (read_filesystem_progress_.IsStopped() ? 0 : 1) : + (is_tablet_server_ ? (start_tablets_progress_.IsStopped() ? 0 : 1) : 0) : + (is_tablet_server_ ? 0 : : (initialize_master_catalog_progress_.IsStopped() ? 0 : 1)) : + (start_rpc_server_progress_.IsStopped() ? 0 : 1); nit: IMO would be more easily read as int counter = 0; counter += init_progress_.IsStopped() ? 0 : 1; counter += read_filesystem_progress_.IsStopped() ? 0 : 1; if (is_tablet_server_) { counter += start_tablets_progress_.IsStopped() ? 0 : 1; } else { counter += initialize_master_catalog_progress_.IsStopped() ? 0 : 1; } counter += start_rpc_server_progress_.IsStopped() ? 0 : 1; as is done below http://gerrit.cloudera.org:8080/#/c/17903/5/src/kudu/server/startup_path_handler.cc@152 PS5, Line 152: int64_t nit: consider returning a MonoDelta, and doing the millisecond conversion at the callsite -- To view, visit http://gerrit.cloudera.org:8080/17903 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a508c3baf0a0d77baf75f36f7bb305a6ad821e1 Gerrit-Change-Number: 17903 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 27 Oct 2021 01:38:18 +0000 Gerrit-HasComments: Yes
