You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/09/15 22:06:54 UTC

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle 

Tim Armstrong has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8069

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

IMPALA-5895: clean up runtime profile lifecycle

TODO: cleanup bucketing counters?

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 437 insertions(+), 410 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/4
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

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


Patch Set 7:

(1 comment)

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_);
> #ifndef NDEBUG
We shouldn't take locks only under debug builds since they have side effects (w.r.t. synchronization) and so can hide bugs from debug builds.


-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

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


Patch Set 10: Code-Review+1

I went over the fix to AddTimeSeriesCounter, and it looks correct. This patch LGTM.

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

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


Patch Set 7: Code-Review+2

Please let Sailesh finish his review as well.

-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 12: Code-Review+2

rebase onto the rat check fix to unblock merging

-- 
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

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


Patch Set 11: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1237/

-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

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


Patch Set 12:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1239/

-- 
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#7).

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

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 450 insertions(+), 416 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/7
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.

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


IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Consolidate 'time_series_counter_map_lock_' and 'counter_map_lock_'
to reduce complexity of the locking scheme. I didn't see any benefit
to sharding the locks in this way - there are only two time series
counters per fragment instance, which a small fraction of the
total number of counters.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Reviewed-on: http://gerrit.cloudera.org:8080/8069
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 458 insertions(+), 428 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

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


Patch Set 9: Code-Review+1

(2 comments)

LGTM, except these 2 comments.

http://gerrit.cloudera.org:8080/#/c/8069/9/be/src/exec/data-source-scan-node.cc
File be/src/exec/data-source-scan-node.cc:

PS9, Line 368:   PeriodicCounterUpdater::StopRateCounter(total_throughput_counter());
             :   PeriodicCounterUpdater::StopTimeSeriesCounter(bytes_read_timeseries_counter_);
Spurious. These should be stopped in ScanNode::Close() ?


http://gerrit.cloudera.org:8080/#/c/8069/9/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

PS9, Line 244: PeriodicCounterUpdater::StopTimeSeriesCounter(
             :       recvr_->bytes_received_time_series_counter_);
Seems like an outlier.


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 11: Code-Review+2

-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/8069

to look at the new patch set (#8).

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

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 451 insertions(+), 416 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/8
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/8069

to look at the new patch set (#10).

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

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Consolidate 'time_series_counter_map_lock_' and 'counter_map_lock_'
to reduce complexity of the locking scheme. I didn't see any benefit
to sharding the locks in this way - there are only two time series
counters per fragment instance, which a small fraction of the
total number of counters.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 458 insertions(+), 428 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/10
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

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


Patch Set 11:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1237/

-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8069/8//COMMIT_MSG
Commit Message:

Line 19: 
> Could you document that you removed the time_series_counter_map_lock_ and t
Done


http://gerrit.cloudera.org:8080/#/c/8069/8/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

PS8, Line 178: profile_
> How come we don't stop counters for this profile? There are other profiles 
There aren't any periodic counters registered in any data sinks so it doesn't make a difference. If someone adds such a counter in the future they're responsible for adding the StopPeriodicCounters() call. The DCHECK enforces this.


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

PS8, Line 950: vector<RuntimeProfile::Counter*>* RuntimeProfile::AddBucketingCounters(Counter* src_counter,
> Long line
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#6).

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

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 442 insertions(+), 417 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/6
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil 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_);
#ifndef NDEBUG
    lock_guard<SpinLock> l(counter_map_lock_);
    DCHECK(!has_active_periodic_counters_);
#endif

For correctness?


PS7, Line 88: void RuntimeProfile::StopPeriodicCounters() {
This doesn't stop the counters that belong to child profiles. Is it on the user to stop counters of child profiles?


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_


PS7, Line 408: sampling_counters
nit: sampling_counters_


-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/8069

to look at the new patch set (#9).

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

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Consolidate 'time_series_counter_map_lock_' and 'counter_map_lock_'
to reduce complexity of the locking scheme. I didn't see any benefit
to sharding the locks in this way - there are only two time series
counters per fragment instance, which a small fraction of the
total number of counters.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 451 insertions(+), 416 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/9
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8069/9/be/src/exec/data-source-scan-node.cc
File be/src/exec/data-source-scan-node.cc:

PS9, Line 368:   PeriodicCounterUpdater::StopRateCounter(total_throughput_counter());
             :   PeriodicCounterUpdater::StopTimeSeriesCounter(bytes_read_timeseries_counter_);
> Spurious. These should be stopped in ScanNode::Close() ?
Good point, I missed those.


http://gerrit.cloudera.org:8080/#/c/8069/9/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

PS9, Line 244: PeriodicCounterUpdater::StopTimeSeriesCounter(
             :       recvr_->bytes_received_time_series_counter_);
> Seems like an outlier.
Good point. I released that we didn't actually call StopPeriodicCounters() on the profile_ here, which made me realise there was a bug in AddTimeSeriesCounter() where the flag wasn't set.


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

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


Patch Set 12: Verified+1

-- 
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#5).

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

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 437 insertions(+), 410 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/5
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

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


Patch Set 10: Code-Review+2

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 10:

Fix wasn't totally trivial so would be good to have you check my work

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

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


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8069/8//COMMIT_MSG
Commit Message:

Line 19: 
Could you document that you removed the time_series_counter_map_lock_ and the reasoning?


http://gerrit.cloudera.org:8080/#/c/8069/8/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

PS8, Line 178: profile_
How come we don't stop counters for this profile? There are other profiles in other files where we don't stop them too.

Wouldn't that be a slight change in behavior? Since they were previously stopped in the destructor?


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

PS8, Line 950: vector<RuntimeProfile::Counter*>* RuntimeProfile::AddBucketingCounters(Counter* src_counter,
Long line


-- 
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes