Tim Armstrong has posted comments on this change. Change subject: IMPALA-5275: Avoid printing status stack trace on hot paths ......................................................................
Patch Set 2: Code-Review+1 (1 comment) Looks good, this should be a big improvement. There's an additional nice-to-have but I won't block this if it's non-trivial. http://gerrit.cloudera.org:8080/#/c/7449/2/be/src/common/status.cc File be/src/common/status.cc: Line 48: Status::Status(TErrorCode::type code) It would be nice to have versions of Status::Expected() that took the predefined error codes, since we should be encouraging use of those instead of unstructured strings. It would add a lot of boilerplate to stamp out all the variants manually, but it looks it's possible to forward multiple arguments in C++11: https://stackoverflow.com/a/2821244. We could probably get ride of the below boilerplate too, sicne they are all just forwarding the arguments to ErrorMsg() I'm ok with the current code if that isn't easily doable but it would be nice to do the cleanup. -- To view, visit http://gerrit.cloudera.org:8080/7449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ief083f558fba587381aa7fe8f99da279da02f1f2 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
