Dan Hecht has posted comments on this change. Change subject: IMPALA-5895: clean up runtime profile lifecycle ......................................................................
Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: PS6, Line 476: Open Open() PS6, Line 480: Initialized previous comment you say "created". is it a synonym (and if so, let's common-ize). http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: PS6, Line 73: We "we" seems ambiguous. is this talking about subclasses as well or just this class? the header comment seems to imply the former, but then at least some subclasses also do this, right? http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/scan-node.h File be/src/exec/scan-node.h: Line 87: virtual Status Prepare(RuntimeState* state) WARN_UNUSED_RESULT; nit: i think we usually have newline between method comments http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/runtime/data-stream-recvr.cc File be/src/runtime/data-stream-recvr.cc: PS6, Line 267: profile_ is that still the expected one now that profile is the child? http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/util/periodic-counter-updater.h File be/src/util/periodic-counter-updater.h: PS6, Line 73: the counter what is "the counter"? http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: PS6, Line 330: with the index corresponding : /// to the value how is the index computed from the src_counter value? is the src_counter value used as the index directly, or is it scaled in some way? i guess i'm asking if the bucket width is 1 or can it be > 1? PS6, Line 336: AddBucketingCounters the other Add functions are given a 'name'. why doesn't this one need a name? PS6, Line 383: owned by counter_map_ but then the counter_map_ comment seems to say that they are owned by the profile. which is it? PS6, Line 391: typedef std::map<std::string, TimeSeriesCounter*> TimeSeriesCounterMap; : TimeSeriesCounterMap time_series_counter_map_; comment PS6, Line 398: counter_child_map_ child_counter_map_? PS6, Line 479: non-locked that sounds like they aren't protected by the lock. Maybe "non-locking"? Or say "Internal implementations of the Add*Counter() ..." -- To view, visit http://gerrit.cloudera.org:8080/8069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-HasComments: Yes
