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

Reply via email to