Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12129 )
Change subject: IMPALA-6664: Tag log statements with fragment ids. ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/12129/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12129/1//COMMIT_MSG@31 PS1, Line 31: TODO: there's no test here yet... I'll wait for this before +2ing. http://gerrit.cloudera.org:8080/#/c/12129/1/be/src/common/logging.cc File be/src/common/logging.cc: http://gerrit.cloudera.org:8080/#/c/12129/1/be/src/common/logging.cc@55 PS1, Line 55: std:: std:: not needed http://gerrit.cloudera.org:8080/#/c/12129/1/be/src/common/logging.cc@59 PS1, Line 59: if (changed != nullptr) { It seems reasonable to assume that 'changed' is non-NULL. http://gerrit.cloudera.org:8080/#/c/12129/1/be/src/common/logging.cc@65 PS1, Line 65: // Manipulates log messages by: The API chosen is a little unfortunate since it requires reallocating 's' to prepend anything. Looking at the glog patch, it wouldn't be that hard to provide an alternative way to return a prefix. I guess the overhead probably isn't enough to worry about, and it looks like the inefficiencies in the glog patch haven't shown up in any profiling, it just offends my sensibilities a bit :). http://gerrit.cloudera.org:8080/#/c/12129/1/be/src/common/logging.cc@68 PS1, Line 68: std: same here -- To view, visit http://gerrit.cloudera.org:8080/12129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6634ef9d1a7346339f24f2d40a7a3aa36a535da8 Gerrit-Change-Number: 12129 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Thu, 03 Jan 2019 17:56:51 +0000 Gerrit-HasComments: Yes
