impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5275: Avoid printing status stack trace on hot paths
Date Wed, 19 Jul 2017 22:15:30 GMT
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 <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@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