Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201709182157.v8ILv1ZB012690@ip-10-146-233-104.ec2.internal>
Date: Mon, 18 Sep 2017 21:57:01 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5895=3A_clean_up_runtime_profile_lifecycle=0A?=
In-Reply-To: <gerrit.1505373167000.I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df@gerrit.cloudera.org>
References: <gerrit.1505373167000.I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Mon, 18 Sep 2017 21:57:08 -0000

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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes