KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/21356 )
Change subject: [metrics] Add metrics for tablet copy op time ...................................................................... Patch Set 9: (6 comments) Thanks for your reviews. http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tablet/tablet_metrics.cc@227 PS8, Line 227: _source, > Would 'Tablet Copy Duration' be good enough? Done http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tablet/tablet_metrics.cc@229 PS8, Line 229: > Does it make sense to mention this is the duration as seen from the source Done http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tablet/tablet_metrics.cc@229 PS8, Line 229: :kMilliseco > tablet copying Done http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tablet/tablet_metrics.cc@231 PS8, Line 231: kudu::Metr > Yingchun already pointed at that in PS5, but it seems there is still room f Done 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)); : > Does it make sense to verify the corresponding metric both at the destinati Yes, this metric is designed to be effective only on the source tablet during the copy execution process and will not be displayed on the target shard. This is primarily determined by the copy process, as there are additional operations after the copy execution is completed to ensure that the target tablet transitions to a normal working state. Updating the metric before this transition seems meaningless. http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: http://gerrit.cloudera.org:8080/#/c/21356/8/src/kudu/tserver/tablet_copy_source_session.cc@514 PS8, Line 514: PREDICT_TRUE(metrics != nullptr)) { : int64_t elapsed_ms = (MonoTime::Now() - start_time_ > In tablet_metrics.cc, the tablet_copy_duration is defined in microseconds i Done -- 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: 9 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: Mon, 20 May 2024 03:43:34 +0000 Gerrit-HasComments: Yes
