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 <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
