Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201709192008.v8JK8Mlw005330@ip-10-146-233-104.ec2.internal>
Date: Tue, 19 Sep 2017 20:08:22 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Sailesh Mukil <sailesh@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: tarmstrong@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: Tue, 19 Sep 2017 20:08:26 -0000

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5895: clean up runtime profile lifecycle
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8069/7/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS7, Line 85: DCHECK(!has_active_periodic_counters_);
> We shouldn't take locks only under debug builds since they have side effect
Also the destructor can't safely run concurrently with other method calls regardless.


PS7, Line 88: void RuntimeProfile::StopPeriodicCounters() {
> This doesn't stop the counters that belong to child profiles. Is it on the 
Yeah, my thinking was that whatever added the counters to the profile should also explicitly stop them. Updated the method comment since it wasn't very clear.


http://gerrit.cloudera.org:8080/#/c/8069/7/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

PS7, Line 407: rate_counters
> nit: rate_counters_
Done


PS7, Line 408: sampling_counters
> nit: sampling_counters_
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes