Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19479 )
Change subject: [KUDU-3447] Limit tablets copying speed ...................................................................... Patch Set 18: (16 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 Done http://gerrit.cloudera.org:8080/#/c/19479/17/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19479/17/src/kudu/tools/kudu-tool-test.cc@9361 PS17, Line 9361: ASSERT_GT(source_tserver_tablet_count, 0); > Better to use ASSERT_GT(), the actual value of source_tserver_tablet_count Done http://gerrit.cloudera.org:8080/#/c/19479/17/src/kudu/tools/kudu-tool-test.cc@9381 PS17, Line 9381: 10 * FLAGS_tablet_copy_transfer_chunk_size_byt > nit: leave a space on both left and right sides of '*'. Done 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. Done 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 = std::make_shared<Throttler>(MonoTi > Please use std::make_shared() here instead. Done http://gerrit.cloudera.org:8080/#/c/19479/17/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/19479/17/src/kudu/tools/tool_action_local_replica.cc@334 PS17, Line 334: shared_ptr<Throttler> throttler; : if (FLAGS_tablet_copy_throttler_bytes_per_sec > 0) { : throttler = std::make_shared<Throttler>(MonoTime::Now(), : 0, : FLAGS_tablet_copy_throttler_bytes_per_sec, : FLAGS_tablet_copy_throttler_burst_factor); : } : > The throttler is only used in CLI tool? How about create a throttler intern I doubt different business has different limiting speed. Can replicas migration/auto rebalancing share the same throttler? 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 Done http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@270 PS11, Line 270: : RaftPeerPB* cstate_leader; : ConsensusStatePB cstate; : RETURN_NOT_OK(tablet_replica_->consensus()->ConsensusState(&cstate)); : RETURN_NOT_OK(GetRaftConfigLeader(&cstate, &cstate_leader)); : leader_ = *cstate_leader; : return Status::OK(); : } : : Status TabletCopyClientTest::ResetLocalTabletCopyClient() { : // client_ will be reset many times in test cases, we only shutdown mini_server_ : // at the first time, and will not start it again. : if (mini_server_->is_started()) { : > Is it possible to pass tablet_copy_client_metrics_.get() and throttler_ as Done http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@337 PS11, Line 337: ASSERT_OK(StartCopy()); : BlockId block_id = FirstColumnBlockId(*client_->remote_superblock_); : Slice slice; : faststring scratch; : : // Ensure the block wasn't there before (it shouldn't be, we use our own FsManager dir). : Status s = ReadLocalBlockFile(fs_manager_.get(), block_id, &scratch, & > Is it possible to move this into the initializers' list? Sub-class can not initialize the member filed of parent class in the initializers list of sub-class. it will throw error: error: class ‘kudu::tserver::TabletCopyThrottlerTest’ does not have any field named ‘mode_’. http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@347 PS11, Line 347: ockId new_ > I'm not sure this is needed -- isn't gtest checking for any errors from Set It does not. Please see the function SetUp() in class TabletCopyClientTest, which maybe throw errors. http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@368 PS11, Line 368: > nit: this seems to be an incomplete sentence Done http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@369 PS11, Line 369: STANTIATE_TEST_SUITE_P(TabletCopyClientBasicTestModes, TabletCopyClien > With integer rounding, is this stable enough to not introduce any flakiness Done 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 It is shared by multiple tablet copying tasks in the same session. 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: // A throttler to limit tablet copy speed. > Can we make it a 'std::unique_ptr'? No. throttler is shared by multiple tablet copying tasks in the same tablet server. We want to limit the copying speed of all tablets belonging to one copy_from_remote command, not only one. 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 copying speed. It limit the copying speed of all the tablets " : "in one server for one session. The default value is 0, which means not limit " : "the speed. The unit is bytes/seconds"); : DEFINE_int64(tablet_copy_throttler_burst_factor, 0, : "Burst factor > Are these definitions necessary here? They are already defined in tool_acti Here defines the parameters, tool_action_local_replica.cc only declare the parameters. Please check again. 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 copying spe > It would be great to include more details in this description, in particula Done -- 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: 18 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, 16 Oct 2023 09:11:23 +0000 Gerrit-HasComments: Yes
