Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19479 )
Change subject: [KUDU-3447] Limit tablets copying speed ...................................................................... Patch Set 22: (20 comments) http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tools/kudu-tool-test.cc@9360 PS20, Line 9360: > nit for here and elsewhere in this new test scenario: introducing a variabl Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tools/kudu-tool-test.cc@9377 PS20, Line 9377: > nit: maybe, drop this extra space? Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tools/kudu-tool-test.cc@9381 PS20, Line 9381: > style nit: please move these parameters into their own line(s) Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tools/kudu-tool-test.cc@9390 PS20, Line 9390: _cluster_->mini_tablet_server(target_idx)->Start()); : const vector<string>& target_tablet_ids > nit: probably, using FindOrNull() from map-util.h would be more readable? target_tablet_ids is a vector. 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@347 PS11, Line 347: > That NO_FATALS() in TabletCopyClientTest::SetUp() around TabletCopyTest::Se Got it. Thanks for your explaination. http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@335 PS20, Line 335: table > nit: does 'tool' is relevant in this test? Change it to 'tablet-copy-test'. http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@337 PS20, Line 337: ASSERT_OK(ResetRemoteTabletCopyClient(&tablet_copy_client > I meant 'consider wrapping this into ASSERT_OK()' Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@356 PS20, Line 356: must > nit: must be Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@357 PS20, Line 357: FLAGS_tablet_copy_transfer_chunk_size_bytes > This doesn't seem to be or right dimension, no? Shouldn't the time that ha Please see line 324, I have set the tablet copying speed to FLAGS_tablet_copy_transfer_chunk_size_bytes. http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@361 PS20, Line 361: 'client_ > nit: 'client_' Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@361 PS20, Line 361: d > nit: remove this single quote symbol? Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@362 PS20, Line 362: d u > uses Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@362 PS20, Line 362: the pointer > a pointer to Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@363 PS20, Line 363: while being destr > while being destroyed Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@363 PS20, Line 363: t.cc. > It's going to be confusing in the future when another change in tablet_copy Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.h@328 PS20, Line 328: pub > style nit: the indent should be 2 spaces 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: // The throttler_ is shared among mult > Great, then please add a comment for this field, please: Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.cc@84 PS20, Line 84: limit > nit: limits Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.cc@85 PS20, Line 85: not limiting " : "the speed > nit: 'not limiting the speed' or 'there isn't any limit' Done http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.cc@87 PS20, Line 87: > Does 0 represent a special value for the semantics of this flag? If so, pl OK. The default value of tablet_copy_throttler_burst_factor should be better to be 1.0, which means the maximun rate is equals "to tablet_copy_throttler_bytes_per_sec". I will change it. -- 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: 22 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: Wed, 25 Oct 2023 06:07:06 +0000 Gerrit-HasComments: Yes
