Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21356 )
Change subject: [metrics] Add metrics for tablet copy op time ...................................................................... Patch Set 10: (7 comments) Almost there! Just a few nits. http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tablet/tablet_metrics.h File src/kudu/tablet/tablet_metrics.h: http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tablet/tablet_metrics.h@114 PS10, Line 114: tablet_copy_duration_as_source nit: the details are available in the description of the metric (and it's propagated into the auto-generated documentation), so this could be just tablet_copy_duration http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tablet/tablet_metrics.cc@255 PS10, Line 255: tablet_copy_duration_as_source nit: the details are available in the description, so this could be just tablet_copy_duration http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tablet/tablet_metrics.cc@256 PS10, Line 256: Tablet Copy Operation Duration As Source Tablet nit: the details are available in the description, so this could be just Tablet Copy Duration http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tablet/tablet_metrics.cc@258 PS10, Line 258: Duration of the tablet copying operation as source tablet. Duration of tablet copying when using this tablet replica as a source. http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tserver/tablet_copy_service-test.cc File src/kudu/tserver/tablet_copy_service-test.cc: http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tserver/tablet_copy_service-test.cc@237 PS8, Line 237: // Test the metric that tracks the duration of a tablet copy session on source side. : TEST_F(TabletCopyMetricTest, TestRunTimeMetricFinishState) { : const auto before_cnt = CopyRunTime()->TotalCount(); : string session_id; : ASSERT_OK(DoBeginValidTabletCopySession(&session_id)); : : EndTabletCopySessionResponsePB resp; : RpcController controller; : ASSERT_OK(DoEndTabletCopySession(session_id, true, nullptr, &resp, &controller)); : > Yes, this metric is designed to be effective only on the source tablet duri Yep, this makes sense. My question was whether we want to track similar metric at the target replica, making sure it stays zero after the copying completed and the target replica is up and running. http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tserver/tablet_copy_service-test.cc File src/kudu/tserver/tablet_copy_service-test.cc: http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tserver/tablet_copy_service-test.cc@237 PS10, Line 237: on source side nit: at the source side http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: http://gerrit.cloudera.org:8080/#/c/21356/10/src/kudu/tserver/tablet_copy_source_session.cc@512 PS10, Line 512: if (PREDICT_TRUE(tablet_replica_->tablet())) Is there any situaion when RemoteTabletCopySourceSession::UpdateTabletMetrics() could be called when tablet_replica_->tablet() is nullptr? Do you mind adding a comment to explain why this if() clause is needed? Thank you! -- To view, visit http://gerrit.cloudera.org:8080/21356 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I088f6a9a8a07ad39ca95ae8b4995ce00d1a0d00c Gerrit-Change-Number: 21356 Gerrit-PatchSet: 10 Gerrit-Owner: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Wed, 29 May 2024 20:13:44 +0000 Gerrit-HasComments: Yes
