Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8256 )
Change subject: KUDU-2187. Don't hold threadpool lock while creating threads ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/thread.cc File src/kudu/util/thread.cc: http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/thread.cc@532 PS2, Line 532: if (PREDICT_FALSE(FLAGS_thread_inject_start_latency_ms > 0)) { Could we use MAYBE_INJECT_LATENCY from fault_injection.h instead? http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/threadpool-test.cc@185 PS2, Line 185: TEST_F(ThreadPoolTest, TestThreadPoolWithNoMinimum) { Perhaps this test (and others that used num_threads_) no longer need friendship? http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/threadpool-test.cc@361 PS2, Line 361: // Now have the "submitter" threads submit 10 tasks. Each task Having some difficulties with this comment: 1. It's easy to misinterpret the group of "10 tasks" as being the "outer" tasks, but here you're actually intending for it to refer to the "inner" tasks. 2. "This" in "This should trigger..." could use a little more explanation. Perhaps "This is long enough that it should trigger..."? http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/threadpool-test.cc@389 PS2, Line 389: ASSERT_LE(total_queue_time_ms, 10000); Does this hold in an "adversarial" environment (i.e. TSAN, looping, stress threads, etc.)? http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/threadpool.cc@477 PS2, Line 477: num_threads_, max_threads_, total_queued_tasks_, max_queue_size_)); May want to add num_threads_pending_start_ here. http://gerrit.cloudera.org:8080/#/c/8256/2/src/kudu/util/threadpool.cc@721 PS2, Line 721: RETURN_NOT_OK(kudu::Thread::Create("thread pool", strings::Substitute("$0 [worker]", name_), : &ThreadPool::DispatchThread, this, &t)); : return Status::OK(); Nit: could combine. Could also replace 't' with nullptr. -- To view, visit http://gerrit.cloudera.org:8080/8256 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If91cb032db25ed539ec8a952f302cf4501b3c240 Gerrit-Change-Number: 8256 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 11 Oct 2017 18:12:44 +0000 Gerrit-HasComments: Yes
