Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19479 )
Change subject: [KUDU-3447] Limit tablets copying speed ...................................................................... Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tools/tool_action_local_replica.cc@336 PS11, Line 336: throttler.reset(new Throttler(MonoTime::Now(), Please use std::make_shared() here instead. http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@259 PS11, Line 259: nit: remove the extra spaces http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@270 PS11, Line 270: if (use_throttler_) { : client_.reset(new RemoteTabletCopyClient(GetTabletId(), : fs_manager_.get(), : cmeta_manager, : messenger_, : tablet_copy_client_metrics_.get(), : throttler_)); : } else { : client_.reset(new RemoteTabletCopyClient(GetTabletId(), : fs_manager_.get(), : cmeta_manager, : messenger_, : nullptr /* no metrics */)); : } Is it possible to pass tablet_copy_client_metrics_.get() and throttler_ as the last two arguments instead of introducing this if/else clause? http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@337 PS11, Line 337: mode_ = TabletCopyMode::REMOTE; : use_throttler_ = true; : throttler_.reset( : new Throttler(MonoTime::Now(), : 0, : FLAGS_tablet_copy_transfer_chunk_size_bytes, : 2 * FLAGS_tablet_copy_transfer_chunk_size_bytes)); Is it possible to move this into the initializers' list? Also, please use std::make_shared() to create a new instance of std::shared_ptr. http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@347 PS11, Line 347: NO_FATALS( I'm not sure this is needed -- isn't gtest checking for any errors from SetUp() on itself? http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@368 PS11, Line 368: Tablet copy speed must less than throttler defined speed nit: this seems to be an incomplete sentence http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@369 PS11, Line 369: ASSERT_GE(FLAGS_tablet_copy_transfer_chunk_size_bytes, current_speed); With integer rounding, is this stable enough to not introduce any flakiness? Have you tried to run this test scenario with, say --stress_cpu_threads=16 under RELEASE, DEBUG, ASAN, and TSAN builds? http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client.h@321 PS11, Line 321: std::shared_ptr<Throttler> throttler_; Why to use std::shared_ptr here? Please describe what components share the ownership. If it's not shared ownership, then consider using std::unique_ptr instead. http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client.cc@84 PS11, Line 84: Limit tablet copy speed. It would be great to include more details in this description, in particular: * Is this per-session limit (i.e. it limits the bandwidth of just a single table copying) or that's server-wide limit (i.e. this limits the total bandwidth of all tablet copying sessions at this tablet server)? * What's the semantics of value 0? -- 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: 11 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> 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: Mon, 09 Oct 2023 19:57:49 +0000 Gerrit-HasComments: Yes
