impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3567: move ExecOption profile helpers to RuntimeProfile
Date Tue, 06 Sep 2016 20:22:45 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message