You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2020/03/16 09:19:55 UTC
[kudu-CR] remove kudu::Thread from tests
Hello Alexey Serbin, Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15447
to review the following change.
Change subject: remove kudu::Thread from tests
......................................................................
remove kudu::Thread from tests
std::thread is much more ergonomic, evidenced in part by the silly
kudu::Thread names and categories found in tests.
I retained kudu::Thread in debug-util-test and trace-test, both of which
rely on some internal Kudu threading detail. And of course in thread-test
which actually exercises this functionality.
Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/benchmarks/wal_hiccup.cc
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/client/client-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/tablet_copy_client_session-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/update_scan_delta_compact-test.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/mini-cluster/webui_checker.cc
M src/kudu/mini-cluster/webui_checker.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_random_access-test.cc
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/countdown_latch-test.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/mt-hdr_histogram-test.cc
M src/kudu/util/mt-metrics-test.cc
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/striped64-test.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/test_graph.h
49 files changed, 597 insertions(+), 778 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/15447/1
--
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: newchange
Gerrit-Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
Gerrit-Change-Number: 15447
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] remove kudu::Thread from tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15447
to look at the new patch set (#4).
Change subject: remove kudu::Thread from tests
......................................................................
remove kudu::Thread from tests
std::thread is much more ergonomic, evidenced in part by the silly
kudu::Thread names and categories found in tests.
I retained kudu::Thread in debug-util-test and trace-test, both of which
rely on some internal Kudu threading detail. And of course in thread-test
which actually exercises this functionality.
Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/benchmarks/wal_hiccup.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/client/client-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/tablet_copy_client_session-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/update_scan_delta_compact-test.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/mini-cluster/webui_checker.cc
M src/kudu/mini-cluster/webui_checker.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_random_access-test.cc
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/countdown_latch-test.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/mt-hdr_histogram-test.cc
M src/kudu/util/mt-metrics-test.cc
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/striped64-test.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/test_graph.h
48 files changed, 618 insertions(+), 792 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/15447/4
--
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: newpatchset
Gerrit-Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
Gerrit-Change-Number: 15447
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] remove kudu::Thread from tests
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15447 )
Change subject: remove kudu::Thread from tests
......................................................................
Patch Set 4: Code-Review+2
--
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: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 17 Mar 2020 23:52:40 +0000
Gerrit-HasComments: No
[kudu-CR] remove kudu::Thread from tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15447
to look at the new patch set (#3).
Change subject: remove kudu::Thread from tests
......................................................................
remove kudu::Thread from tests
std::thread is much more ergonomic, evidenced in part by the silly
kudu::Thread names and categories found in tests.
I retained kudu::Thread in debug-util-test and trace-test, both of which
rely on some internal Kudu threading detail. And of course in thread-test
which actually exercises this functionality.
Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/benchmarks/wal_hiccup.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/client/client-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/tablet_copy_client_session-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/update_scan_delta_compact-test.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/mini-cluster/webui_checker.cc
M src/kudu/mini-cluster/webui_checker.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_random_access-test.cc
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/countdown_latch-test.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/mt-hdr_histogram-test.cc
M src/kudu/util/mt-metrics-test.cc
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/striped64-test.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/test_graph.h
48 files changed, 608 insertions(+), 783 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/15447/3
--
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: newpatchset
Gerrit-Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
Gerrit-Change-Number: 15447
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] remove kudu::Thread from tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15447 )
Change subject: remove kudu::Thread from tests
......................................................................
remove kudu::Thread from tests
std::thread is much more ergonomic, evidenced in part by the silly
kudu::Thread names and categories found in tests.
I retained kudu::Thread in debug-util-test and trace-test, both of which
rely on some internal Kudu threading detail. And of course in thread-test
which actually exercises this functionality.
Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
Reviewed-on: http://gerrit.cloudera.org:8080/15447
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/benchmarks/wal_hiccup.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/client/client-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/tablet_copy_client_session-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/update_scan_delta_compact-test.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/mini-cluster/webui_checker.cc
M src/kudu/mini-cluster/webui_checker.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_random_access-test.cc
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/countdown_latch-test.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/mt-hdr_histogram-test.cc
M src/kudu/util/mt-metrics-test.cc
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/striped64-test.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/test_graph.h
48 files changed, 618 insertions(+), 792 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
Gerrit-Change-Number: 15447
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] remove kudu::Thread from tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15447 )
Change subject: remove kudu::Thread from tests
......................................................................
Patch Set 3:
(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 ?
Done
http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/fs/block_manager-test.cc@1099
PS3, Line 1099: const
> ditto
Done
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.
Ack
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?
Done
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.
Ack
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) ?
Done
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 removin
Good idea. Done.
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?
Done
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 a
Done
http://gerrit.cloudera.org:8080/#/c/15447/3/src/kudu/rpc/mt-rpc-test.cc@289
PS3, Line 289: const
> nit: constexpr?
Done
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?
Done
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_EVENTUA
Done
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?
Done
--
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 17 Mar 2020 22:26:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] remove kudu::Thread from tests
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15447
to look at the new patch set (#2).
Change subject: remove kudu::Thread from tests
......................................................................
remove kudu::Thread from tests
std::thread is much more ergonomic, evidenced in part by the silly
kudu::Thread names and categories found in tests.
I retained kudu::Thread in debug-util-test and trace-test, both of which
rely on some internal Kudu threading detail. And of course in thread-test
which actually exercises this functionality.
Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
---
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/benchmarks/wal_hiccup.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/client/client-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/create-table-itest.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_election-itest.cc
M src/kudu/integration-tests/tablet_copy_client_session-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/integration-tests/update_scan_delta_compact-test.cc
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/mini-cluster/webui_checker.cc
M src/kudu/mini-cluster/webui_checker.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_random_access-test.cc
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/countdown_latch-test.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/mt-hdr_histogram-test.cc
M src/kudu/util/mt-metrics-test.cc
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/once-test.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/striped64-test.cc
M src/kudu/util/test_graph.cc
M src/kudu/util/test_graph.h
48 files changed, 608 insertions(+), 784 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/15447/2
--
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: newpatchset
Gerrit-Change-Id: I39b87fc3d8542143507edf79a5ca05a7fc27ffd4
Gerrit-Change-Number: 15447
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] remove kudu::Thread from tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15447 )
Change subject: remove kudu::Thread from tests
......................................................................
Patch Set 4: Code-Review+2
--
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: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 17 Mar 2020 22:49:40 +0000
Gerrit-HasComments: No
[kudu-CR] remove kudu::Thread from tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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