Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15447 )
Change subject: remove kudu::Thread from tests ...................................................................... Patch Set 3: Code-Review+2 (13 comments) http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/fs/block_manager-test.cc@814 PS3, Line 814: const nit: constexpr ? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/fs/block_manager-test.cc@1099 PS3, Line 1099: const ditto http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/alter_table-test.cc@1201 PS3, Line 1201: SCOPED_CLEANUP({ : stop_threads_ = true; : for (auto& t : threads) { : t.join(); : } : }); Good catch! Thank you for adding this proper clean-up. http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/client-stress-test.cc File src/kudu/integration-tests/client-stress-test.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/client-stress-test.cc@156 PS3, Line 156: const int: add constexpr? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/consistency-itest.cc@843 PS3, Line 843: this is a broken use of a PRNG as each call to Next32() : // yield the same value. Fixing it is non-trivial because 'first_row' is : // multiplied in various code paths, causing it to overflow an int32. It : // just so happens that the first Next32() using this particular seed : // generates a low enough value to avoid overflow Whoops. Yep, it's better to address it in a separate changelist. http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/create-table-itest.cc File src/kudu/integration-tests/create-table-itest.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/create-table-itest.cc@482 PS3, Line 482: vector<thread> threads; nit: add threads.reserve(16) ? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/tablet_copy_client_session-itest.cc File src/kudu/integration-tests/tablet_copy_client_session-itest.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/integration-tests/tablet_copy_client_session-itest.cc@177 PS3, Line 177: auto thread_join_func = [&]() { : for (auto& t : threads) { : t.join(); : } : }; : auto thread_join = MakeScopedCleanup(thread_join_func); : : // Restart the source tablet server (TS 0). : ASSERT_OK(cluster_->tablet_server(0)->Restart()); : : // Wait for one of the threads to succeed with its tablet copy and for the : // tablet to be running on TS 1. : ASSERT_OK(WaitUntilTabletRunning(ts1, tablet_id, kTimeout)); : : thread_join_func(); : thread_join.cancel(); nit: maybe, wrapping this into a sub-scope using SCOPED_CLEANUP and removing the last two calls would be cleaner? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/rpc/mt-rpc-test.cc File src/kudu/rpc/mt-rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/rpc/mt-rpc-test.cc@118 PS3, Line 118: const constexpr? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/rpc/mt-rpc-test.cc@227 PS3, Line 227: 3 nit: maybe, it's worth introducing a constexpr for number of threads here as well? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/rpc/mt-rpc-test.cc@289 PS3, Line 289: const nit: constexpr? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/rpc/rpc-test.cc@1470 PS3, Line 1470: const constexpr? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/util/maintenance_manager-test.cc File src/kudu/util/maintenance_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/util/maintenance_manager-test.cc@245 PS3, Line 245: thread.join(); maybe, create a SCOPED_CLEANUP to take care of the thread if ASSERT_EVENTUALLY above fails? http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/util/once-test.cc File src/kudu/util/once-test.cc: http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/util/once-test.cc@115 PS3, Line 115: const nit: constexpr? -- To view, visit http://gerrit.cloudera.org:8080/15447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4 Gerrit-Change-Number: 15447 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 17 Mar 2020 19:22:45 +0000 Gerrit-HasComments: Yes
