Tim Armstrong 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() Done PS6, Line 480: Initialized > previous comment you say "created". is it a synonym (and if so, let's commo Done 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 Done 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 Done 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? It looks like the child_profile was added recently as part of IMPALA-5773. I think it was just an oversight that this pointed to profile_ instead of child_profile. Seems more consistent to have everything in the same profile. 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"? Clarified that it's 'buckets'. 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 v Done PS6, Line 336: AddBucketingCounters > the other Add functions are given a 'name'. why doesn't this one need a nam Added a comment with further explanation. These "bucketing" counters are only used in one place in HdfsScanNode. The counters doesn't actually show up in the profile, they're processed and added as a string in the profile. This is seriously wonky but it doesn't seem worth tackling right now. PS6, Line 383: owned by counter_map_ > but then the counter_map_ comment seems to say that they are owned by the p Comment was inaccurate. Change just to say that they also appear in counter_map. PS6, Line 391: typedef std::map<std::string, TimeSeriesCounter*> TimeSeriesCounterMap; : TimeSeriesCounterMap time_series_counter_map_; > comment Done PS6, Line 398: counter_child_map_ > child_counter_map_? Done PS6, Line 479: non-locked > that sounds like they aren't protected by the lock. Maybe "non-locking"? O Done -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
