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 <tmarshall@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
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 @@
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 @@
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 @@
// 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 @@
/// 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.
