impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
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:


Running a private test to make sure none of the tests break due to specific expectations of
Status behavior:
File be/src/common/

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.

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.
File be/src/service/

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.
File be/src/util/error-util.h:

PS3, Line 116: string
> const reference

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d9d63610eb0d2acae3a9303ce46e1410727ce87
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message