Todd Lipcon has posted comments on this change. Change subject: log: shut down appender thread when idle ......................................................................
Patch Set 1: (11 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? yea, would have to be a whole second, but lemme see if I can make it less flaky-prone with a loop 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. Done Line 229: gscoped_ptr<ThreadPool> append_pool_; > Given this design and given that the append thread is blocked on IO when ru yea, I don't know if there's a lot of benefit in doing that... suppose it would be possible at some point but this seemed easier and we still get the end goal of no threads running for cold tablets Line 243: .set_idle_timeout(MonoDelta::FromSeconds(0)) > Add a comment for this? I'm guessing it's because we handle idleness oursel Done Line 248: void Log::AppendThread::Wake() { > DCHECK(append_pool_) maybe? Done 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 hard to repro in a test without injecting various sleeps. I did manually inject sleeps in the right places to cover all the code paths, and we do have some multi-threaded tests like mt-log-test as well as any multi-threaded integration tests covering this. Do you think it's worth polluting the code with MAYBE_INJECT_RANDOM_LATENCY() calls to trigger potential races? Line 288: void Log::AppendThread::DoWork() { > Maybe DCHECK that worker_state_ is WORKER_ACTIVE? Done Line 365: append_pool_->Wait(); > Why call Wait() prior to Shutdown()? IIRC, Shutdown() will wait for outstan I don't think we want to cancel the pending ones, because then I think we'll end up orphaning entries in the blocking queue. We want the task to drain the queue and run the callbacks, I believe. (The previous behavior by joining is doing the same thing, allowing the appender thread to drain the queue) http://gerrit.cloudera.org:8080/#/c/6856/1/src/kudu/util/blocking_queue-test.cc File src/kudu/util/blocking_queue-test.cc: Line 31: using std::shared_ptr; > warning: using decl 'shared_ptr' is unused [misc-unused-using-decls] Done 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 Done 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? Done -- 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
