Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17903 )
Change subject: KUDU-1959 - Implement server startup progress page for tablet and master servers ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/17903/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17903/2//COMMIT_MSG@7 PS2, Line 7: Implement server startup progress page for tablet and master servers > Was under the impression all the patches should have the same header. Will While the headers may reuse jira numbers, each patch should have unique descriptions describing just the contents of the patch. See https://github.com/apache/kudu/search?q=kudu-2612&type=commits for several different patches in a series. http://gerrit.cloudera.org:8080/#/c/17903/2/src/kudu/server/startup_path_handler.cc File src/kudu/server/startup_path_handler.cc: http://gerrit.cloudera.org:8080/#/c/17903/2/src/kudu/server/startup_path_handler.cc@142 PS2, Line 142: : int32 StartupPathHandler::StartupProgressPercentageMetric() { : int percent = 0; : percent += (init_progress_.IsStopped()) ? 100 : 0; : percent += (read_filesystem_progress_.IsStopped()) ? 100 : 0; : percent += (start_rpc_server_progress_.IsStopped()) ? 100 : 0; : if (is_tablet_server_) { : if (containers_total_ == 0 && read_data_directories_progress_.IsStopped()) { : percent += 100; : } else if (containers_total_ != 0) { : percent += containers_processed_ * 100 / containers_total_; : } : } else { : percent += (initialize_master_catalog_progress_.IsStopped()) ? 100 : 0; : } : return percent/4; : } > True, but since we have the below metric, we would want to know if the star I see. Another approach would be to use METRIC_state() to define a counter for the stage/state the startup has completed already. http://gerrit.cloudera.org:8080/#/c/17903/2/src/kudu/server/startup_path_handler.cc@159 PS2, Line 159: : int64_t StartupPathHandler::StartupProgressTimeElapsedMetric() { : int64_t time_elapsed = 0; : time_elapsed += init_progress_.TimeElapsed().ToMilliseconds(); : time_elapsed += read_filesystem_progress_.TimeElapsed().ToMilliseconds(); : if (is_tablet_server_) { : time_elapsed += start_tablets_progress_.TimeElapsed().ToMilliseconds(); : } else { : time_elapsed += initialize_master_catalog_progress_.TimeElapsed().ToMilliseconds(); : } : time_elapsed += start_rpc_server_progress_.TimeElapsed().ToMilliseconds(); : return time_elapsed; : } : : > Right, this won't be super useful while the server is starting up, but will Makes sense. Please add that reasoning to the commit message, i.e. the "why" https://chris.beams.io/posts/git-commit/#why-not-how http://gerrit.cloudera.org:8080/#/c/17903/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/17903/2/src/kudu/tserver/ts_tablet_manager.cc@1381 PS2, Line 1381: ++*tablets_processed; : tablets_num_opened_startup_->Increment(); > So the metric will be instantiated only after all the tablets have been sta I see your point. Though while it seems useful, maybe it's not _that_ useful. In that case, let's make these all debug-level metrics. -- 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: 3 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, 13 Oct 2021 19:13:46 +0000 Gerrit-HasComments: Yes
