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-3342: Add thread counters to monitor plan fragment execution
Date Thu, 20 Oct 2016 23:40:07 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment execution
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4633/6//COMMIT_MSG
Commit Message:

Line 9: This change removes the use of total_cpu_timer which incorrectly
> Do you mean provide a snippet of the profile in the commit message?
Yes


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 138
Why don't we create the thread counters here like before? This seems much cleaner rather than
having PlanFragmentExecutor do it.


http://gerrit.cloudera.org:8080/#/c/4633/6/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 242:   RuntimeProfile::ThreadCounters* plan_fragment_counters_;
> We access this object  variable in plan-fragment-executor.Hence I kept it p
The convention is that the trailing underscore means that the member is private. Generally
all class members should be private and exposed if needed via accessor functions.

Sometimes we use friend classes if two classes are very tightly coupled by design.


-- 
To view, visit http://gerrit.cloudera.org:8080/4633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <aphadke@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message