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-5895: clean up runtime profile lifecycle
Date Mon, 18 Sep 2017 23:14:11 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message