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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Yonghyun Hwang Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
