impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5198: Error messages are sometimes dropped before reaching client
Date Sat, 15 Apr 2017 01:00:59 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5198: Error messages are sometimes dropped before reaching client
......................................................................


Patch Set 3:

(5 comments)

Running a private test to make sure none of the tests break due to specific expectations of
Status behavior:

http://sandbox.jenkins.cloudera.com/view/Impala/view/Private-Utility/job/impala-private-build-and-test/5465/

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 
That's true. But I'd rather not make the change in this patch, since there are some micro-optimizations
done, regarding the 'delete' operator, etc. which will take some time to go through.
https://github.com/apache/incubator-impala/blob/master/be/src/common/status.h#L170
https://github.com/apache/incubator-impala/blob/master/be/src/common/status.h#L254

We could probably do it as later clean up if necessary.


PS3, Line 216:       std::for_each(status.error_msgs.begin() + 1, status.error_msgs.end(),
             :           [&](string const& detail) { msg_->AddDetail(detail); });
> fancy
Ha. Read that this in many cases is better in perf than a regular for loop with iterators,
not that it would matter that much here.


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
Yes, because of duplication.

I can run a private test with the latest patch to see if all the tests pass to make sure.


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 e
Oops, forgot to remove that. Done.


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
Done


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