Todd Lipcon 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? that one randomizes the latency, but for this test it's better that it be fixed. 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 friend Was able to remove only one of the friends. The other tests use active_threads_ still 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: rewrote the comment 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 this allows for it to be 20x slower than normal. I ran it successfully with 100 stress threads (on an 88-core box). So, I think it's OK. If not I"ll tweak it if it becomes flaky. 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. Done 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. Done -- 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: Fri, 13 Oct 2017 00:58:38 +0000 Gerrit-HasComments: Yes
