IMPALA-5198: Error messages are sometimes dropped before reaching client The Status::ToThrift() function takes the ErrorMsg, and pushes both the msg() and details() into the TStatus::error_msgs list.
However, when we unpack the TStatus object into a Status object, we just copy all the TStatus::error_msgs to Status::ErrorMsg::details_ and leave Status::ErrorMsg::message_ blank. This led to the error message not being printed in certain cases which is now fixed. The PlanFragmentExecutor had some code to add query statuses to the error_log (IMP-633), which is no longer necessary after a future patch (IMPALA-762) explicitly returned the query status to the client via get_log(), making the adding of the query statuses to the error_log redundant. That code in the PFE has been removed and a test has been added to make sure that the case it previously tried to fix doesn't regress. Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87 Reviewed-on: http://gerrit.cloudera.org:8080/6627 Reviewed-by: Sailesh Mukil <[email protected]> Tested-by: Impala Public 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/e0d1db51 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/e0d1db51 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/e0d1db51 Branch: refs/heads/master Commit: e0d1db51ed4627cbe47fcec0d9a2909d14abbe4a Parents: fb2a785 Author: Sailesh Mukil <[email protected]> Authored: Wed Apr 12 23:53:42 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Thu Apr 20 22:58:56 2017 +0000 ---------------------------------------------------------------------- be/src/common/status.cc | 28 +++++++++++++++++++-------- be/src/common/status.h | 3 +++ be/src/runtime/plan-fragment-executor.cc | 7 ------- be/src/util/error-util.h | 10 ++++++---- tests/data_errors/test_data_errors.py | 23 +++++++++++++++++++++- 5 files changed, 51 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e0d1db51/be/src/common/status.cc ---------------------------------------------------------------------- diff --git a/be/src/common/status.cc b/be/src/common/status.cc index 3d3e6e7..e3b821c 100644 --- a/be/src/common/status.cc +++ b/be/src/common/status.cc @@ -136,17 +136,13 @@ Status::Status(const string& error_msg, bool silent) Status::Status(const ErrorMsg& message) : msg_(new ErrorMsg(message)) { } -Status::Status(const TStatus& status) - : msg_(status.status_code == TErrorCode::OK - ? NULL : new ErrorMsg(status.status_code, status.error_msgs)) { } +Status::Status(const TStatus& status) { + FromThrift(status); +} Status& Status::operator=(const TStatus& status) { delete msg_; - if (status.status_code == TErrorCode::OK) { - msg_ = NULL; - } else { - msg_ = new ErrorMsg(status.status_code, status.error_msgs); - } + FromThrift(status); return *this; } @@ -212,6 +208,22 @@ void Status::ToThrift(TStatus* status) const { } } +void Status::FromThrift(const TStatus& status) { + if (status.status_code == TErrorCode::OK) { + msg_ = NULL; + } else { + msg_ = new ErrorMsg(); + msg_->SetErrorCode(status.status_code); + if (status.error_msgs.size() > 0) { + // The first message is the actual error message. (See Status::ToThrift()). + msg_->SetErrorMsg(status.error_msgs.front()); + // The following messages are details. + std::for_each(status.error_msgs.begin() + 1, status.error_msgs.end(), + [&](string const& detail) { msg_->AddDetail(detail); }); + } + } +} + void Status::FreeMessage() noexcept { delete msg_; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e0d1db51/be/src/common/status.h ---------------------------------------------------------------------- diff --git a/be/src/common/status.h b/be/src/common/status.h index ff54dc9..a3e44e0 100644 --- a/be/src/common/status.h +++ b/be/src/common/status.h @@ -252,6 +252,9 @@ class Status { // A non-inline function for freeing status' message. void FreeMessage() noexcept; + /// A non-inline function for unwrapping a TStatus object. + void FromThrift(const TStatus& status); + /// Status uses a naked pointer to ensure the size of an instance on the stack is only /// the sizeof(ErrorMsg*). Every Status owns its ErrorMsg instance. ErrorMsg* msg_; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e0d1db51/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 3594138..df5b0d2 100644 --- a/be/src/runtime/plan-fragment-executor.cc +++ b/be/src/runtime/plan-fragment-executor.cc @@ -335,13 +335,6 @@ Status PlanFragmentExecutor::Exec() { SCOPED_TIMER(profile()->total_time_counter()); SCOPED_TIMER(ADD_TIMER(timings_profile_, EXEC_TIMER_NAME)); status = ExecInternal(); - - if (!status.ok() && !status.IsCancelled() && !status.IsMemLimitExceeded()) { - // Log error message in addition to returning in Status. Queries that do not fetch - // results (e.g. insert) may not receive the message directly and can only retrieve - // the log. - runtime_state_->LogError(status.msg()); - } } FragmentComplete(status); return status; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e0d1db51/be/src/util/error-util.h ---------------------------------------------------------------------- diff --git a/be/src/util/error-util.h b/be/src/util/error-util.h index 06441cb..f245c2c 100644 --- a/be/src/util/error-util.h +++ b/be/src/util/error-util.h @@ -83,9 +83,6 @@ class ErrorMsg { const ArgType& arg5, const ArgType& arg6, const ArgType& arg7, const ArgType& arg8, const ArgType& arg9); - ErrorMsg(TErrorCode::type error, const std::vector<string>& detail) - : error_(error), details_(detail) {} - /// Static initializer that is needed to avoid issues with static initialization order /// and the point in time when the string list generated via thrift becomes /// available. This method should not be used if no static initialization is needed as @@ -111,10 +108,15 @@ class ErrorMsg { } /// Set a specific error code. - void SetError(TErrorCode::type e) { + void SetErrorCode(TErrorCode::type e) { error_ = e; } + /// Set a specific error message. + void SetErrorMsg(const std::string& msg) { + message_ = msg; + } + /// Return the formatted error string. const std::string& msg() const { return message_; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e0d1db51/tests/data_errors/test_data_errors.py ---------------------------------------------------------------------- diff --git a/tests/data_errors/test_data_errors.py b/tests/data_errors/test_data_errors.py index 51add81..92ae707 100644 --- a/tests/data_errors/test_data_errors.py +++ b/tests/data_errors/test_data_errors.py @@ -22,6 +22,7 @@ import pytest import random +from tests.beeswax.impala_beeswax import ImpalaBeeswaxException from tests.common.impala_test_suite import ImpalaTestSuite from tests.common.skip import SkipIfS3, SkipIfLocal from tests.common.test_dimensions import create_exec_option_dimension @@ -41,6 +42,27 @@ class TestDataErrors(ImpalaTestSuite): def get_workload(self): return 'functional-query' +# Regression test for IMP-633. Added as a part of IMPALA-5198 +class TestHdfsFileOpenFailErrors(ImpalaTestSuite): + @pytest.mark.execute_serially + def test_hdfs_file_open_fail(self): + absolute_location = "/test-warehouse/file_open_fail" + create_stmt = \ + "create table file_open_fail (x int) location '" + absolute_location + "'" + insert_stmt = "insert into file_open_fail values(1)" + select_stmt = "select * from file_open_fail" + drop_stmt = "drop table if exists file_open_fail purge" + self.client.execute(drop_stmt) + self.client.execute(create_stmt) + self.client.execute(insert_stmt) + self.filesystem_client.delete_file_dir(absolute_location, recursive=True) + assert not self.filesystem_client.exists(absolute_location) + try: + self.client.execute(select_stmt) + except ImpalaBeeswaxException as e: + assert "Failed to open HDFS file" in str(e) + self.client.execute(drop_stmt) + @SkipIfS3.qualified_path class TestHdfsScanNodeErrors(TestDataErrors): @@ -59,7 +81,6 @@ class TestHdfsScanNodeErrors(TestDataErrors): pytest.xfail("Expected results differ across file formats") self.run_test_case('DataErrorsTest/hdfs-scan-node-errors', vector) - @SkipIfS3.qualified_path @SkipIfLocal.qualified_path class TestHdfsSeqScanNodeErrors(TestHdfsScanNodeErrors):
