Dan Hecht has posted comments on this change. Change subject: IMPALA-3567: move ExecOption profile helpers to RuntimeProfile ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/4188/3/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: PS3, Line 465: || does the clang formatter put this at the start of lines? this is opposite of what we do almost everywhere. http://gerrit.cloudera.org:8080/#/c/4188/3/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: PS3, Line 207: Status Given that this function doesn't have anything to do with "Status" class, how about renaming it "AddCodegenMsg" or something similar? Also, not this commit, but should we eventually always require 'extra_info' to be passed? does it ever make sense to not give a reason when !codegen_enabled? Or do we plan to phase out this variant and always require a Status object once we always plumb it back? PS3, Line 211: OK is OK allowed even when !codegen_enabled? Also, the bool and the Status seem redundant, why do we need both? -- To view, visit http://gerrit.cloudera.org:8080/4188 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21c1dda8f8a1d92172bf59fbc1070a6834e61913 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
