Adar Dembo has posted comments on this change. Change subject: log: shut down appender thread when idle ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/6856/1/src/kudu/consensus/log-test.cc File src/kudu/consensus/log-test.cc: Line 1105: ASSERT_TRUE(log_->append_thread_active_for_tests()); Could be flaky if enough time passes between Append() and now, right? http://gerrit.cloudera.org:8080/#/c/6856/1/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: Line 203: // Tries to transition back to WORKER_STOPPED state. If successful, returns true. Nit: Add a blank line before. Line 229: gscoped_ptr<ThreadPool> append_pool_; Given this design and given that the append thread is blocked on IO when running, I take it we don't want to consolidate pools across tablets? Line 243: .set_idle_timeout(MonoDelta::FromSeconds(0)) Add a comment for this? I'm guessing it's because we handle idleness ourselves, so there's no reason for the threadpool to keep a thread alive if we've deemed it unnecessary. Line 248: void Log::AppendThread::Wake() { DCHECK(append_pool_) maybe? Line 256: // Stopping is a bit tricky. We have to consider the following race: Do we have any unit tests that can semi-reliably produce this race? A multi-threaded stress test for the log? Line 288: void Log::AppendThread::DoWork() { Maybe DCHECK that worker_state_ is WORKER_ACTIVE? Line 365: append_pool_->Wait(); Why call Wait() prior to Shutdown()? IIRC, Shutdown() will wait for outstanding tasks and cancel all the pending tasks; isn't that what we want? http://gerrit.cloudera.org:8080/#/c/6856/1/src/kudu/util/blocking_queue-test.cc File src/kudu/util/blocking_queue-test.cc: Line 64: ASSERT_OK(test_queue.BlockingDrainTo(&out, MonoTime::Now() + MonoDelta::FromSeconds(5))); May be flaky if enough time passes between creation of the deadline and the call to BlockingDrainTo. Maybe make it obnoxiously higher, like 30s? http://gerrit.cloudera.org:8080/#/c/6856/1/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: Line 89: // timeout: How long we'll keep around an idle thread before timing it out. Change this to idle_timeout too? -- To view, visit http://gerrit.cloudera.org:8080/6856 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id40a4cfcec96198b537c2f50be7ff1204faf96d2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
