Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19479 )
Change subject: [KUDU-3447] Limit tablets copying speed ...................................................................... Patch Set 20: (19 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@9381 PS20, Line 9381: nullptr, &stderr) style nit: please move these parameters into their own line(s) http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tools/kudu-tool-test.cc@9390 PS20, Line 9390: find(target_tablet_ids.begin(), target_tablet_ids.end(), tablet_id) : != target_tablet_ids.end() nit: probably, using FindOrNull() from map-util.h would be more readable? 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@337 PS11, Line 337: ResetRemoteTabletCopyClient(&tablet_copy_client_metrics); : : ASSERT_OK(StartCopy()); : BlockId block_id = FirstColumnBlockId(*client_->remote_superblock_); : Slice slice; : faststring scratch; : > Sub-class can not initialize the member filed of parent class in the initia Indeed -- for some reason I assumed these fields were from the TabletCopyThrottlerTest class. http://gerrit.cloudera.org:8080/#/c/19479/11/src/kudu/tserver/tablet_copy_client-test.cc@347 PS11, Line 347: > It does not. Please see the function SetUp() in class TabletCopyClientTest, That NO_FATALS() in TabletCopyClientTest::SetUp() around TabletCopyTest::SetUp() is to bail out of the function/method if error happened in TabletCopyTest::SetUp(), not continue executing the rest of the code of the TabletCopyClientTest::SetUp() method. As I could see, gtest checks on the errors raised during SetUp() on itself, and if any error present, no further phases after SetUp() are run and test considered failed. With that, NO_FATALS() isn't needed here since there isn't anything else in this SetUp() method except for the call of TabletCopyClientTest::SetUp(). 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: tool- nit: does 'tool' is relevant in this test? http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@337 PS20, Line 337: ResetRemoteTabletCopyClient(&tablet_copy_client_metrics); What this into ASSERT_OK()? http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@356 PS20, Line 356: must nit: must be 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 has been spent also be accounted for here? http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@361 PS20, Line 361: 'client_ nit: 'client_' http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@361 PS20, Line 361: ' nit: remove this single quote symbol? http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@362 PS20, Line 362: use uses http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@362 PS20, Line 362: the point of a pointer to http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@363 PS20, Line 363: when be destroyed while being destroyed http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client-test.cc@363 PS20, Line 363: line 231. It's going to be confusing in the future when another change in tablet_copy_client.cc updates the code and the line number changes. Please don't refer to the line numbers like this. 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: style nit: the indent should be 2 spaces 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_; > It is shared by multiple tablet copying tasks in the same session. Great, then please add a comment for this field, please: 'The throttler_ is shared among multiple tablet copying tasks in the same session.' 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 http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.cc@85 PS20, Line 85: not limit " : "the speed nit: 'not limiting the speed' or 'there isn't any limit' http://gerrit.cloudera.org:8080/#/c/19479/20/src/kudu/tserver/tablet_copy_client.cc@87 PS20, Line 87: 0 Does 0 represent a special value for the semantics of this flag? If so, please mention that in the flag description. -- 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: 20 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 24 Oct 2023 16:44:44 +0000 Gerrit-HasComments: Yes
