Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/common/status.cc File be/src/common/status.cc: PS3, Line 210: new ErrorMsg(); not really relevant to your change, but I wonder if there's any reason why we don't keep this in a unique_ptr so we don't have to explicitly delete it in the destructor. PS3, Line 216: std::for_each(status.error_msgs.begin() + 1, status.error_msgs.end(), : [&](string const& detail) { msg_->AddDetail(detail); }); fancy http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: PS3, Line 288: : : : the consistency issue aside, why remove this? is it because the information is duplicated in the GetErrorLog() output? are we sure we don't have any tests that rely on the order in which this info is printed? PS3, Line 293: if (!exec_state->query_status().ok()) error_log_ss << "\n\n"; I'm not sure if this still makes sense, if the query status is !ok you'll end up with a few extra newlines at the top of the returned string. http://gerrit.cloudera.org:8080/#/c/6627/3/be/src/util/error-util.h File be/src/util/error-util.h: PS3, Line 116: string const reference -- To view, visit http://gerrit.cloudera.org:8080/6627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
