[impala] branch master updated: IMPALA-9267: Fix DCHECK in ClientRequestState::UpdateNonErrorExecState

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stakiar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git


The following commit(s) were added to refs/heads/master by this push:
     new a36e2c9  IMPALA-9267: Fix DCHECK in ClientRequestState::UpdateNonErrorExecState
a36e2c9 is described below

commit a36e2c9c5eb313f5fddb08a52382ca3237397706
Author: Sahil Takiar <ta...@gmail.com>
AuthorDate: Wed Jan 8 09:43:21 2020 -0800

    IMPALA-9267: Fix DCHECK in ClientRequestState::UpdateNonErrorExecState
    
    Fixes a DCHECK in ClientRequestState::UpdateNonErrorExecState where the
    ClientRequestState ExecState attempts to transition from the ERROR to
    the FINISHED state. The DCHECK was added in IMPALA-6894 in order to
    prevent any invalid state transition attempts.
    
    The fix is to modify UpdateNonErrorExecState so that it skips any
    attempt to transition from the ERROR to the FINISHED state, which is in
    line with the behavior prior to IMPALA-6894.
    
    Testing:
    * Ran core tests, unable to reproduce the original issue locally
    
    Change-Id: Ie47444ed67704d9469310727eeec2e9a66516e77
    Reviewed-on: http://gerrit.cloudera.org:8080/14991
    Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/service/client-request-state.cc | 30 +++++++++++++++++++-----------
 be/src/service/client-request-state.h  |  3 ++-
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc
index 023bddc..1bdcfcf 100644
--- a/be/src/service/client-request-state.cc
+++ b/be/src/service/client-request-state.cc
@@ -830,6 +830,9 @@ void ClientRequestState::Wait() {
     if (stmt_type() == TStmtType::DDL) {
       DCHECK(catalog_op_type() != TCatalogOpType::DDL || request_result_set_ != nullptr);
     }
+    // It is possible the query already failed at this point and ExecState is ERROR. In
+    // this case, the call to UpdateNonErrorExecState(FINISHED) does not change the
+    // ExecState.
     UpdateNonErrorExecState(ExecState::FINISHED);
   }
   // UpdateQueryStatus() or UpdateNonErrorExecState() have updated exec_state_.
@@ -925,11 +928,12 @@ Status ClientRequestState::RestartFetch() {
 void ClientRequestState::UpdateNonErrorExecState(ExecState new_state) {
   lock_guard<mutex> l(lock_);
   ExecState old_state = exec_state_.Load();
-  static string error_msg = "Illegal state transition: $0 -> $1";
+  static string error_msg = "Illegal state transition: $0 -> $1, query_id=$3";
   switch (new_state) {
     case ExecState::PENDING:
-      DCHECK(old_state == ExecState::INITIALIZED) << Substitute(
-          error_msg, ExecStateToString(old_state), ExecStateToString(new_state));
+      DCHECK(old_state == ExecState::INITIALIZED)
+          << Substitute(error_msg, ExecStateToString(old_state),
+              ExecStateToString(new_state), PrintId(query_id()));
       UpdateExecState(new_state);
       break;
     case ExecState::RUNNING:
@@ -940,18 +944,22 @@ void ClientRequestState::UpdateNonErrorExecState(ExecState new_state) {
         // DDL statements and metadata ops don't use the PENDING state, so a query can
         // transition directly from the INITIALIZED to RUNNING state.
         DCHECK(old_state == ExecState::INITIALIZED || old_state == ExecState::PENDING)
-            << Substitute(
-                error_msg, ExecStateToString(old_state), ExecStateToString(new_state));
+            << Substitute(error_msg, ExecStateToString(old_state),
+                ExecStateToString(new_state), PrintId(query_id()));
         UpdateExecState(new_state);
       }
       break;
     case ExecState::FINISHED:
-      // A query can transition from PENDING to FINISHED if it is cancelled by the
-      // client.
-      DCHECK(old_state == ExecState::PENDING || old_state == ExecState::RUNNING)
-          << Substitute(
-              error_msg, ExecStateToString(old_state), ExecStateToString(new_state));
-      UpdateExecState(new_state);
+      // Only transition to the FINISHED state if the query has not failed. It is not
+      // valid to transition from ERROR to FINISHED, so skip any attempt to do so.
+      if (old_state != ExecState::ERROR) {
+        // A query can transition from PENDING to FINISHED if it is cancelled by the
+        // client.
+        DCHECK(old_state == ExecState::PENDING || old_state == ExecState::RUNNING)
+            << Substitute(error_msg, ExecStateToString(old_state),
+                ExecStateToString(new_state), PrintId(query_id()));
+        UpdateExecState(new_state);
+      }
       break;
     default:
       DCHECK(false) << "A non-error state expected but got: "
diff --git a/be/src/service/client-request-state.h b/be/src/service/client-request-state.h
index 3584e67..ac901a4 100644
--- a/be/src/service/client-request-state.h
+++ b/be/src/service/client-request-state.h
@@ -149,7 +149,8 @@ class ClientRequestState {
   /// Update operation state if the requested state isn't already obsolete. This is
   /// only for non-error states (PENDING, RUNNING and FINISHED) - if the query encounters
   /// an error the query status needs to be set with information about the error so
-  /// UpdateQueryStatus() must be used instead. Takes lock_.
+  /// UpdateQueryStatus() must be used instead. If an invalid state transition is
+  /// attempted, this method either DCHECKs or skips the state update. Takes lock_.
   void UpdateNonErrorExecState(ExecState exec_state);
 
   /// Update the query status and the "Query Status" summary profile string.