IMPALA-4383: Ensure plan fragment report thread is always started PlanFragmentExecutor::report_thread_active_ was set during OpenInternal() after ReportProfile() - run in a different thread - signalled that it was running.
Howeer, ReportProfile() reads report_thread_active_ to determine if it should exit its loop. If it runs fast enough, ReportProfile() will never see report_thread_active_ == true. This patch moves setting report_thread_active_ to ReportProfile() - slightly earlier, but visible at almost the same time because it is now correctly protected by report_thread_lock_. Testing: * TestRPCTimeout would hit this in certain configurations. Ran test_execplanfragment_timeout() for 90 minutes with no failures; previously it would fail within the first five attempts repeatedly. Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b Reviewed-on: http://gerrit.cloudera.org:8080/4864 Reviewed-by: Henry Robinson <[email protected]> Tested-by: Henry Robinson <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/d82411f8 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/d82411f8 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/d82411f8 Branch: refs/heads/master Commit: d82411f81c746ec5df2f659e97e6b3ba4472676c Parents: 2808b84 Author: Henry Robinson <[email protected]> Authored: Wed Oct 26 02:50:07 2016 -0700 Committer: Henry Robinson <[email protected]> Committed: Fri Oct 28 19:40:32 2016 +0000 ---------------------------------------------------------------------- be/src/runtime/plan-fragment-executor.cc | 2 +- be/src/runtime/plan-fragment-executor.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d82411f8/be/src/runtime/plan-fragment-executor.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/plan-fragment-executor.cc b/be/src/runtime/plan-fragment-executor.cc index bfb80ea..d079c36 100644 --- a/be/src/runtime/plan-fragment-executor.cc +++ b/be/src/runtime/plan-fragment-executor.cc @@ -313,7 +313,6 @@ Status PlanFragmentExecutor::OpenInternal() { // make sure the thread started up, otherwise ReportProfile() might get into a race // with StopReportThread() report_thread_started_cv_.wait(l); - report_thread_active_ = true; } OptimizeLlvmModule(); @@ -381,6 +380,7 @@ void PlanFragmentExecutor::ReportProfile() { DCHECK(!report_status_cb_.empty()); unique_lock<mutex> l(report_thread_lock_); // tell Open() that we started + report_thread_active_ = true; report_thread_started_cv_.notify_one(); // Jitter the reporting time of remote fragments by a random amount between http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d82411f8/be/src/runtime/plan-fragment-executor.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/plan-fragment-executor.h b/be/src/runtime/plan-fragment-executor.h index 82d3001..6385213 100644 --- a/be/src/runtime/plan-fragment-executor.h +++ b/be/src/runtime/plan-fragment-executor.h @@ -166,7 +166,11 @@ class PlanFragmentExecutor { /// Indicates that profile reporting thread started. /// Tied to report_thread_lock_. boost::condition_variable report_thread_started_cv_; - bool report_thread_active_; // true if we started the thread + + /// When the report thread starts, it sets 'report_thread_active_' to true and signals + /// 'report_thread_started_cv_'. The report thread is shut down by setting + /// 'report_thread_active_' to false and signalling 'stop_report_thread_cv_'. + bool report_thread_active_; /// true if Close() has been called bool closed_;
