[4/5] incubator-impala git commit: IMPALA-4383: Ensure plan fragment report thread is always started

[he...@apache.org]

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 <he...@cloudera.com>
Tested-by: Henry Robinson <he...@cloudera.com>


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 <he...@cloudera.com>
Authored: Wed Oct 26 02:50:07 2016 -0700
Committer: Henry Robinson <he...@cloudera.com>
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_;