IMPALA-4037,IMPALA-4038: fix locking during query cancellation

* Refactor the child query handling out of QueryExecState and clarify
  locking rules.
* Avoid holding QueryExecState::lock_ while calling
  Coordinator::Cancel() or ChildQuery::Cancel(), which can both do RPCs
  or acquire ImpalaServer::query_exec_state_map_lock_.
* Fix a potential race between QueryExecState::Exec() and
  QueryExecState::Cancel() where the cancelling thread did an unlocked
  read of the 'coordinator_' field and may not have cancelled the
  coordinator.

Testing:
Ran exhaustive build, ran local stress test for a bit.

Change-Id: Ibe3024803e03595ee69c47759b58e8443d7bd167
Reviewed-on: http://gerrit.cloudera.org:8080/4163
Reviewed-by: Tim Armstrong <tarmstr...@cloudera.com>
Tested-by: Internal Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/0d0c93ec
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/0d0c93ec
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/0d0c93ec

Branch: refs/heads/master
Commit: 0d0c93ec8c4949940ec113192731f2adb66a0c5e
Parents: d76a2b2
Author: Tim Armstrong <tarmstr...@cloudera.com>
Authored: Mon Aug 29 11:10:38 2016 -0700
Committer: Internal Jenkins <cloudera-hud...@gerrit.cloudera.org>
Committed: Thu Sep 22 10:07:52 2016 +0000

----------------------------------------------------------------------
 be/src/runtime/coordinator.h          |   4 +-
 be/src/service/child-query.cc         |  69 ++++++++++++++++
 be/src/service/child-query.h          |  66 +++++++++++++++
 be/src/service/impala-server.cc       |  13 +--
 be/src/service/query-exec-state.cc    | 125 ++++++++++++++++-------------
 be/src/service/query-exec-state.h     |  49 +++++------
 tests/query_test/test_cancellation.py |   5 --
 7 files changed, 227 insertions(+), 104 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0d0c93ec/be/src/runtime/coordinator.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/coordinator.h b/be/src/runtime/coordinator.h
index 0e0b6d5..617ccb9 100644
--- a/be/src/runtime/coordinator.h
+++ b/be/src/runtime/coordinator.h
@@ -246,7 +246,9 @@ class Coordinator {
   /// Keeps track of number of completed ranges and total scan ranges.
   ProgressUpdater progress_;
 
-  /// protects all fields below
+  /// Protects all fields below. This is held while making RPCs, so this lock 
should
+  /// only be acquired if the acquiring thread is prepared to wait for a 
significant
+  /// time.
   boost::mutex lock_;
 
   /// Overall status of the entire query; set to the first reported fragment 
error

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0d0c93ec/be/src/service/child-query.cc
----------------------------------------------------------------------
diff --git a/be/src/service/child-query.cc b/be/src/service/child-query.cc
index 99eb4fe..5b68de4 100644
--- a/be/src/service/child-query.cc
+++ b/be/src/service/child-query.cc
@@ -146,4 +146,73 @@ Status ChildQuery::IsCancelled() {
   return Status::CANCELLED;
 }
 
+ChildQueryExecutor::ChildQueryExecutor() : is_cancelled_(false), 
is_running_(false) {}
+ChildQueryExecutor::~ChildQueryExecutor() {
+  DCHECK(!is_running_);
+}
+
+void ChildQueryExecutor::ExecAsync(vector<ChildQuery>&& child_queries) {
+  DCHECK(!child_queries.empty());
+  lock_guard<SpinLock> lock(lock_);
+  DCHECK(child_queries_.empty());
+  DCHECK(child_queries_thread_.get() == NULL);
+  if (is_cancelled_) return;
+  child_queries_ = move(child_queries);
+  child_queries_thread_.reset(new Thread("query-exec-state", "async child 
queries",
+      bind(&ChildQueryExecutor::ExecChildQueries, this)));
+  is_running_ = true;
+}
+
+void ChildQueryExecutor::ExecChildQueries() {
+  for (ChildQuery& child_query : child_queries_) {
+    // Execute without holding 'lock_'.
+    Status status = child_query.ExecAndFetch();
+    if (!status.ok()) {
+      lock_guard<SpinLock> lock(lock_);
+      child_queries_status_ = status;
+      break;
+    }
+  }
+
+  {
+    lock_guard<SpinLock> lock(lock_);
+    is_running_ = false;
+  }
+}
+
+Status ChildQueryExecutor::WaitForAll(vector<ChildQuery*>* completed_queries) {
+  // Safe to read without lock since we don't call this concurrently with 
ExecAsync().
+  if (child_queries_thread_ == NULL) {
+    DCHECK(!is_running_);
+    return Status::OK();
+  }
+  child_queries_thread_->Join();
+
+  // Safe to read below fields without 'lock_' because they are immutable 
after the
+  // thread finishes.
+  RETURN_IF_ERROR(child_queries_status_);
+  for (ChildQuery& child_query : child_queries_) {
+    completed_queries->push_back(&child_query);
+  }
+  return Status::OK();
+}
+
+void ChildQueryExecutor::Cancel() {
+  {
+    lock_guard<SpinLock> l(lock_);
+    // Prevent more child queries from starting. After this critical section,
+    // 'child_queries_' will not be modified.
+    is_cancelled_ = true;
+    if (!is_running_) return;
+    DCHECK_EQ(child_queries_thread_ == NULL, child_queries_.empty());
+  }
+
+  // Cancel child queries without holding 'lock_'.
+  // Safe because 'child_queries_' and 'child_queries_thread_' are immutable 
after
+  // cancellation.
+  for (ChildQuery& child_query : child_queries_) {
+    child_query.Cancel();
+  }
+  child_queries_thread_->Join();
+}
 }

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0d0c93ec/be/src/service/child-query.h
----------------------------------------------------------------------
diff --git a/be/src/service/child-query.h b/be/src/service/child-query.h
index 2fdcc5a..1c7a20e 100644
--- a/be/src/service/child-query.h
+++ b/be/src/service/child-query.h
@@ -142,6 +142,72 @@ class ChildQuery {
   bool is_cancelled_;
 };
 
+/// Asynchronously executes a set of child queries in a separate thread.
+///
+/// ExecAsync() is called at most once per executor to execute a set of child 
queries
+/// asynchronously. After ExecAsync() is called, either WaitForAll() or 
Cancel() must be
+/// called to ensure that the child queries are no longer executing before 
destroying the
+/// object.
+class ChildQueryExecutor {
+ public:
+  ChildQueryExecutor();
+  ~ChildQueryExecutor();
+
+  /// Asynchronously executes 'child_queries' one by one in a new thread. 
'child_queries'
+  /// must be non-empty. May clear or modify the 'child_queries' arg. Can only 
be called
+  /// once. Does nothing if Cancel() was already called.
+  void ExecAsync(std::vector<ChildQuery>&& child_queries);
+
+  /// Waits for all child queries to complete successfully or with an error. 
Returns a
+  /// non-OK status if a child query fails. Returns OK if ExecAsync() was not 
called,
+  /// Cancel() was called before an error occurred, or if all child queries 
finished
+  /// successfully. If returning OK, populates 'completed_queries' with the 
completed
+  /// queries. Any returned ChildQueries remain owned by the executor. Should 
not be
+  /// called concurrently with ExecAsync(). After WaitForAll() returns, the 
object can
+  /// safely be destroyed.
+  Status WaitForAll(std::vector<ChildQuery*>* completed_queries);
+
+  /// Cancels all child queries and prevents any more from starting. Returns 
once all
+  /// child queries are cancelled, after which the object can safely be 
destroyed. Can
+  /// be safely called concurrently with ExecAsync() or WaitForAll().
+  void Cancel();
+
+ private:
+  /// Serially executes the queries in child_queries_ by calling the child 
query's
+  /// ExecAndWait(). This function blocks until all queries complete and is run
+  /// in 'child_queries_thread_'.
+  /// Sets 'child_queries_status_'.
+  void ExecChildQueries();
+
+  /// Protects all fields below.
+  /// Should not be held at the same time as 'ChildQuery::lock_'.
+  SpinLock lock_;
+
+  /// True if cancellation of child queries has been initiated and no more 
child queries
+  /// should be started.
+  bool is_cancelled_;
+
+  /// True if 'child_queries_thread_' is in the process of executing child 
queries.
+  /// Set to false by 'child_queries_thread_' just before it exits. 
'is_running_' must
+  /// be false when ChildQueryExecutor is destroyed: once execution is started,
+  /// WaitForAll() or Cancel() must be called to ensure the thread exits.
+  bool is_running_;
+
+  /// List of child queries to be executed. Not modified after it is initially 
populated,
+  /// so safe to read without holding 'lock_' if 'is_running_' or 
'is_cancelled_' is
+  /// true, or 'child_queries_thread_' is non-NULL.
+  std::vector<ChildQuery> child_queries_;
+
+  /// Thread to execute 'child_queries_' in. Immutable after the first time it 
is set or
+  /// after 'is_cancelled_' is true.
+  boost::scoped_ptr<Thread> child_queries_thread_;
+
+  /// The status of the child queries. The status is OK iff all child queries 
complete
+  /// successfully. Otherwise, status contains the error of the first child 
query that
+  /// failed (child queries are executed serially and abort on the first 
error).
+  /// Immutable after 'child_queries_thread_' exits
+  Status child_queries_status_;
+};
 }
 
 #endif

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0d0c93ec/be/src/service/impala-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index 1b10aec..d5fd59a 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -995,18 +995,9 @@ Status ImpalaServer::UpdateCatalogMetrics() {
 Status ImpalaServer::CancelInternal(const TUniqueId& query_id, bool 
check_inflight,
     const Status* cause) {
   VLOG_QUERY << "Cancel(): query_id=" << PrintId(query_id);
-  shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, true);
+  shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, false);
   if (exec_state == NULL) return Status("Invalid or unknown query handle");
-  lock_guard<mutex> l(*exec_state->lock(), adopt_lock_t());
-  if (check_inflight) {
-    lock_guard<mutex> l2(exec_state->session()->lock);
-    if (exec_state->session()->inflight_queries.find(query_id) ==
-        exec_state->session()->inflight_queries.end()) {
-      return Status("Query not yet running");
-    }
-  }
-  // TODO: can we call Coordinator::Cancel() here while holding lock?
-  exec_state->Cancel(cause);
+  exec_state->Cancel(check_inflight, cause);
   return Status::OK();
 }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0d0c93ec/be/src/service/query-exec-state.cc
----------------------------------------------------------------------
diff --git a/be/src/service/query-exec-state.cc 
b/be/src/service/query-exec-state.cc
index eb710fb..9b2fc88 100644
--- a/be/src/service/query-exec-state.cc
+++ b/be/src/service/query-exec-state.cc
@@ -64,6 +64,7 @@ ImpalaServer::QueryExecState::QueryExecState(
   : query_ctx_(query_ctx),
     last_active_time_(numeric_limits<int64_t>::max()),
     ref_count_(0L),
+    child_query_executor_(new ChildQueryExecutor),
     exec_env_(exec_env),
     is_block_on_wait_joining_(false),
     session_(session),
@@ -73,6 +74,7 @@ ImpalaServer::QueryExecState::QueryExecState(
     profile_(&profile_pool_, "Query"),  // assign name w/ id after planning
     server_profile_(&profile_pool_, "ImpalaServer"),
     summary_profile_(&profile_pool_, "Summary"),
+    is_cancelled_(false),
     eos_(false),
     query_state_(beeswax::QueryState::CREATED),
     current_batch_(NULL),
@@ -426,9 +428,14 @@ Status ImpalaServer::QueryExecState::ExecQueryOrDmlRequest(
       query_exec_request.fragments[0].partition.type == 
TPartitionType::UNPARTITIONED;
   DCHECK(has_coordinator_fragment || query_exec_request.__isset.desc_tbl);
 
-  schedule_.reset(new QuerySchedule(query_id(), query_exec_request,
-      exec_request_.query_options, &summary_profile_, query_events_));
-  coord_.reset(new Coordinator(exec_request_.query_options, exec_env_, 
query_events_));
+  {
+    lock_guard<mutex> l(lock_);
+    // Don't start executing the query if Cancel() was called concurrently 
with Exec().
+    if (is_cancelled_) return Status::CANCELLED;
+    schedule_.reset(new QuerySchedule(query_id(), query_exec_request,
+        exec_request_.query_options, &summary_profile_, query_events_));
+    coord_.reset(new Coordinator(exec_request_.query_options, exec_env_, 
query_events_));
+  }
   Status status = exec_env_->scheduler()->Schedule(coord_.get(), 
schedule_.get());
 
   {
@@ -462,15 +469,17 @@ Status ImpalaServer::QueryExecState::ExecDdlRequest() {
     TComputeStatsParams& compute_stats_params =
         exec_request_.catalog_op_request.ddl_params.compute_stats_params;
     // Add child queries for computing table and column stats.
+    vector<ChildQuery> child_queries;
     if (compute_stats_params.__isset.tbl_stats_query) {
-      child_queries_.push_back(
+      child_queries.push_back(
           ChildQuery(compute_stats_params.tbl_stats_query, this, 
parent_server_));
     }
     if (compute_stats_params.__isset.col_stats_query) {
-      child_queries_.push_back(
+      child_queries.push_back(
           ChildQuery(compute_stats_params.col_stats_query, this, 
parent_server_));
     }
-    if (child_queries_.size() > 0) ExecChildQueriesAsync();
+
+    if (child_queries.size() > 0) 
child_query_executor_->ExecAsync(move(child_queries));
     return Status::OK();
   }
 
@@ -600,7 +609,15 @@ Status ImpalaServer::QueryExecState::WaitInternal() {
     return Status::OK();
   }
 
-  RETURN_IF_ERROR(WaitForChildQueries());
+  vector<ChildQuery*> child_queries;
+  Status child_queries_status = 
child_query_executor_->WaitForAll(&child_queries);
+  {
+    lock_guard<mutex> l(lock_);
+    RETURN_IF_ERROR(query_status_);
+    RETURN_IF_ERROR(UpdateQueryStatus(child_queries_status));
+  }
+  query_events_->MarkEvent("Child queries finished");
+
   if (coord_.get() != NULL) {
     RETURN_IF_ERROR(coord_->Wait());
     RETURN_IF_ERROR(Expr::Open(output_expr_ctxs_, coord_->runtime_state()));
@@ -608,8 +625,8 @@ Status ImpalaServer::QueryExecState::WaitInternal() {
   }
 
   if (catalog_op_type() == TCatalogOpType::DDL &&
-      ddl_type() == TDdlType::COMPUTE_STATS && child_queries_.size() > 0) {
-    RETURN_IF_ERROR(UpdateTableAndColumnStats());
+      ddl_type() == TDdlType::COMPUTE_STATS && child_queries.size() > 0) {
+    RETURN_IF_ERROR(UpdateTableAndColumnStats(child_queries));
   }
 
   if (!returns_result_set()) {
@@ -820,22 +837,40 @@ Status 
ImpalaServer::QueryExecState::GetRowValue(TupleRow* row, vector<void*>* r
   return Status::OK();
 }
 
-void ImpalaServer::QueryExecState::Cancel(const Status* cause) {
-  // Cancel and close child queries before cancelling parent.
-  for (ChildQuery& child_query: child_queries_) {
-    child_query.Cancel();
-  }
-
-  // If the query is completed or cancelled, no need to cancel.
-  if (eos_ || query_state_ == QueryState::EXCEPTION) return;
+Status ImpalaServer::QueryExecState::Cancel(bool check_inflight, const Status* 
cause) {
+  Coordinator* coord;
+  {
+    lock_guard<mutex> lock(lock_);
+    if (check_inflight) {
+      lock_guard<mutex> session_lock(session_->lock);
+      if (session_->inflight_queries.find(query_id()) ==
+          session_->inflight_queries.end()) {
+        return Status("Query not yet running");
+      }
+    }
 
-  if (cause != NULL) {
-    DCHECK(!cause->ok());
-    UpdateQueryStatus(*cause);
-    query_events_->MarkEvent("Cancelled");
-    DCHECK_EQ(query_state_, QueryState::EXCEPTION);
-  }
-  if (coord_.get() != NULL) coord_->Cancel(cause);
+    // If the query is completed or cancelled, no need to update state.
+    bool already_done = eos_ || query_state_ == QueryState::EXCEPTION;
+    if (!already_done && cause != NULL) {
+      DCHECK(!cause->ok());
+      UpdateQueryStatus(*cause);
+      query_events_->MarkEvent("Cancelled");
+      DCHECK_EQ(query_state_, QueryState::EXCEPTION);
+    }
+    // Get a copy of the coordinator pointer while holding 'lock_'.
+    coord = coord_.get();
+    is_cancelled_ = true;
+  } // Release lock_ before doing cancellation work.
+
+  // Cancel and close child queries before cancelling parent. 'lock_' should 
not be held
+  // because a) ChildQuery::Cancel() calls back into ImpalaServer and b) 
cancellation
+  // involves RPCs and can take quite some time.
+  child_query_executor_->Cancel();
+
+  // Cancel the parent query. 'lock_' should not be held because cancellation 
involves
+  // RPCs and can block for a long time.
+  if (coord != NULL) coord->Cancel(cause);
+  return Status::OK();
 }
 
 Status ImpalaServer::QueryExecState::UpdateCatalog() {
@@ -988,9 +1023,10 @@ void ImpalaServer::QueryExecState::MarkActive() {
   ++ref_count_;
 }
 
-Status ImpalaServer::QueryExecState::UpdateTableAndColumnStats() {
-  DCHECK_GE(child_queries_.size(), 1);
-  DCHECK_LE(child_queries_.size(), 2);
+Status ImpalaServer::QueryExecState::UpdateTableAndColumnStats(
+    const vector<ChildQuery*>& child_queries) {
+  DCHECK_GE(child_queries.size(), 1);
+  DCHECK_LE(child_queries.size(), 2);
   catalog_op_executor_.reset(
       new CatalogOpExecutor(exec_env_, frontend_, &server_profile_));
 
@@ -998,15 +1034,15 @@ Status 
ImpalaServer::QueryExecState::UpdateTableAndColumnStats() {
   // ExecComputeStats(). Otherwise pass in the column stats result.
   TTableSchema col_stats_schema;
   TRowSet col_stats_data;
-  if (child_queries_.size() > 1) {
-    col_stats_schema = child_queries_[1].result_schema();
-    col_stats_data = child_queries_[1].result_data();
+  if (child_queries.size() > 1) {
+    col_stats_schema = child_queries[1]->result_schema();
+    col_stats_data = child_queries[1]->result_data();
   }
 
   Status status = catalog_op_executor_->ExecComputeStats(
       exec_request_.catalog_op_request.ddl_params.compute_stats_params,
-      child_queries_[0].result_schema(),
-      child_queries_[0].result_data(),
+      child_queries[0]->result_schema(),
+      child_queries[0]->result_data(),
       col_stats_schema,
       col_stats_data);
   {
@@ -1030,31 +1066,6 @@ Status 
ImpalaServer::QueryExecState::UpdateTableAndColumnStats() {
   return Status::OK();
 }
 
-void ImpalaServer::QueryExecState::ExecChildQueriesAsync() {
-  DCHECK(child_queries_thread_.get() == NULL);
-  child_queries_thread_.reset(new Thread("query-exec-state", "async child 
queries",
-      bind(&ImpalaServer::QueryExecState::ExecChildQueries, this)));
-}
-
-void ImpalaServer::QueryExecState::ExecChildQueries() {
-  for (int i = 0; i < child_queries_.size(); ++i) {
-    if (!child_queries_status_.ok()) return;
-    child_queries_status_ = child_queries_[i].ExecAndFetch();
-  }
-}
-
-Status ImpalaServer::QueryExecState::WaitForChildQueries() {
-  if (child_queries_thread_.get() == NULL) return Status::OK();
-  child_queries_thread_->Join();
-  {
-    lock_guard<mutex> l(lock_);
-    RETURN_IF_ERROR(query_status_);
-    RETURN_IF_ERROR(UpdateQueryStatus(child_queries_status_));
-  }
-  query_events_->MarkEvent("Child queries finished");
-  return Status::OK();
-}
-
 void ImpalaServer::QueryExecState::ClearResultCache() {
   if (result_cache_ == NULL) return;
   // Update result set cache metrics and mem limit accounting.

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0d0c93ec/be/src/service/query-exec-state.h
----------------------------------------------------------------------
diff --git a/be/src/service/query-exec-state.h 
b/be/src/service/query-exec-state.h
index feb8afe..0a763ff 100644
--- a/be/src/service/query-exec-state.h
+++ b/be/src/service/query-exec-state.h
@@ -124,9 +124,12 @@ class ImpalaServer::QueryExecState {
   /// Cancels the child queries and the coordinator with the given cause.
   /// If cause is NULL, assume this was deliberately cancelled by the user.
   /// Otherwise, sets state to EXCEPTION.
-  /// Caller needs to hold lock_.
   /// Does nothing if the query has reached EOS or already cancelled.
-  void Cancel(const Status* cause = NULL);
+  ///
+  /// Only returns an error if 'check_inflight' is true and the query is not 
yet
+  /// in-flight. Otherwise, proceed and return Status::OK() even if the query 
isn't
+  /// in-flight (for cleaning up after an error on the query issuing path).
+  Status Cancel(bool check_inflight, const Status* cause);
 
   /// This is called when the query is done (finished, cancelled, or failed).
   /// Takes lock_: callers must not hold lock() before calling.
@@ -217,7 +220,16 @@ class ImpalaServer::QueryExecState {
   /// increased, and decreased once that work is completed.
   uint32_t ref_count_;
 
-  boost::mutex lock_;  // protects all following fields
+  /// Executor for any child queries (e.g. compute stats subqueries). Always 
non-NULL.
+  const boost::scoped_ptr<ChildQueryExecutor> child_query_executor_;
+
+  // Protects all following fields. Acquirers should be careful not to hold it 
for too
+  // long, e.g. during RPCs because this lock is required to make progress on 
various
+  // ImpalaServer requests. If held for too long it can block progress of 
client
+  // requests for this query, e.g. query status and cancellation. Furthermore, 
until
+  // IMPALA-3882 is fixed, it can indirectly block progress on all other 
queries.
+  boost::mutex lock_;
+
   ExecEnv* exec_env_;
 
   /// Thread for asynchronously running Wait().
@@ -282,6 +294,7 @@ class ImpalaServer::QueryExecState {
 
   RuntimeProfile::EventSequence* query_events_;
   std::vector<ExprContext*> output_expr_ctxs_;
+  bool is_cancelled_; // if true, Cancel() was called.
   bool eos_;  // if true, there are no more rows to return
   // We enforce the invariant that query_status_ is not OK iff query_state_
   // is EXCEPTION, given that lock_ is held.
@@ -308,16 +321,6 @@ class ImpalaServer::QueryExecState {
   /// Start/end time of the query
   TimestampValue start_time_, end_time_;
 
-  /// List of child queries to be executed on behalf of this query.
-  std::vector<ChildQuery> child_queries_;
-
-  /// Thread to execute child_queries_ in and the resulting status. The status 
is OK iff
-  /// all child queries complete successfully. Otherwise, status contains the 
error of the
-  /// first child query that failed (child queries are executed serially and 
abort on the
-  /// first error).
-  Status child_queries_status_;
-  boost::scoped_ptr<Thread> child_queries_thread_;
-
   /// Executes a local catalog operation (an operation that does not need to 
execute
   /// against the catalog service). Includes USE, SHOW, DESCRIBE, and EXPLAIN 
statements.
   Status ExecLocalCatalogOp(const TCatalogOpRequest& catalog_op);
@@ -333,6 +336,8 @@ class ImpalaServer::QueryExecState {
   /// Core logic of initiating a query or dml execution request.
   /// Initiates execution of plan fragments, if there are any, and sets
   /// up the output exprs for subsequent calls to FetchRows().
+  /// 'coord_' is only valid after this method is called, and may be invalid 
if it
+  /// returns an error.
   /// Also sets up profile and pre-execution counters.
   /// Non-blocking.
   Status ExecQueryOrDmlRequest(const TQueryExecRequest& query_exec_request);
@@ -386,23 +391,7 @@ class ImpalaServer::QueryExecState {
   /// For example, INSERT queries update partition metadata in UpdateCatalog() 
using a
   /// TUpdateCatalogRequest, whereas our DDL uses a TCatalogOpRequest for very 
similar
   /// purposes. Perhaps INSERT should use a TCatalogOpRequest as well.
-  Status UpdateTableAndColumnStats();
-
-  /// Asynchronously executes all child_queries_ one by one. Calls 
ExecChildQueries()
-  /// in a new child_queries_thread_.
-  void ExecChildQueriesAsync();
-
-  /// Serially executes the queries in child_queries_ by calling the child 
query's
-  /// ExecAndWait(). This function is blocking and is intended to be run in a 
separate
-  /// thread to ensure that Exec() remains non-blocking. Sets 
child_queries_status_.
-  /// Must not be called while holding lock_.
-  void ExecChildQueries();
-
-  /// Waits for all child queries to complete successfully or with an error, 
by joining
-  /// child_queries_thread_. Returns a non-OK status if a child query fails or 
if the
-  /// parent query is cancelled (subsequent children will not be executed). 
Returns OK
-  /// if child_queries_thread_ is not set or if all child queries finished 
successfully.
-  Status WaitForChildQueries();
+  Status UpdateTableAndColumnStats(const std::vector<ChildQuery*>& 
child_queries);
 
   /// Sets result_cache_ to NULL and updates its associated metrics and mem 
consumption.
   /// This function is a no-op if the cache has already been cleared.

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0d0c93ec/tests/query_test/test_cancellation.py
----------------------------------------------------------------------
diff --git a/tests/query_test/test_cancellation.py 
b/tests/query_test/test_cancellation.py
index c7cc6da..265c781 100644
--- a/tests/query_test/test_cancellation.py
+++ b/tests/query_test/test_cancellation.py
@@ -72,11 +72,6 @@ class TestCancellation(ImpalaTestSuite):
     # Ignore 'compute stats' queries for the CTAS query type.
     cls.TestMatrix.add_constraint(lambda v: not (v.get_value('query_type') == 
'CTAS' and
          v.get_value('query').startswith('compute stats')))
-    # Ignore debug actions for 'compute stats' because cancellation of 
'compute stats'
-    # relies on child queries eventually making forward progress, but debug 
actions
-    # will cause child queries to hang indefinitely.
-    cls.TestMatrix.add_constraint(lambda v: not (v.get_value('action') == 
'WAIT' and
-         v.get_value('query').startswith('compute stats')))
     # tpch tables are not generated for hbase as the data loading takes a very 
long time.
     # TODO: Add cancellation tests for hbase.
     cls.TestMatrix.add_constraint(lambda v:\

Reply via email to