impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Date Sat, 15 Apr 2017 00:13:32 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message