Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19479 )
Change subject: [KUDU-3447] Limit tablets copying speed ...................................................................... Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tools/kudu-tool-test.cc@9384 PS10, Line 9384: ASSERT_STR_CONTAINS(stderr, "Time spent Tablet copy throttler"); I don't think we should add a new test just to verify the output message or log. Maybe we can collect the number of bytes downloaded and the time used, and to verify the rate is within the limit? http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tools/tool_action_local_replica.cc@1386 PS10, Line 1386: .AddOptionalParameter("tablet_copy_throttler_bytes_per_sec") : .AddOptionalParameter("tablet_copy_throttler_burst_factor") nit: It's necessary to mention these two parameters in the commit message. http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tserver/tablet_copy_client.h@320 PS10, Line 320: std::shared_ptr<Throttler> throttler_; Can we make it a 'std::unique_ptr'? Maybe use it like this: std::unique_ptr<Throttler> throttler_ = nullptr; if (FLAGS_tablet_copy_throttler_bytes_per_sec > 0) throttler.reset(new Throttler()). http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/19479/10/src/kudu/tserver/tablet_copy_client.cc@83 PS10, Line 83: DEFINE_int64(tablet_copy_throttler_bytes_per_sec, 0, : "Limit tablet copy speed."); : DEFINE_int64(tablet_copy_throttler_burst_factor, 0, : "Burst factor for tablet copy throttling. The maximum rate the throttler " : "allows within a token refill period (100ms) equals burst factor multiply " : "base rate."); Are these definitions necessary here? They are already defined in tool_action_local_replica.cc -- To view, visit http://gerrit.cloudera.org:8080/19479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f4834bfb0718a2b6b1d946975287a11f6be1fe3 Gerrit-Change-Number: 19479 Gerrit-PatchSet: 10 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Wed, 27 Sep 2023 12:33:07 +0000 Gerrit-HasComments: Yes
