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 <sailesh@cloudera.com>
Tested-by: Impala Public Jenkins

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_;
 }

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_;

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;

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_;

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):