[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


IMPALA-4678: move query MemTracker into QueryState

The query MemTracker for query execution is now owned directly by
QueryState, which greatly simplifies the lifecycle of the MemTracker.
This required various other changes and enabled some simplifications.

* The coordinator's QueryState is constructed earlier before fragments
  are sent out, since we need a MemTracker at that point.
* The global query MemTracker map can be removed.
* The static request pool mem tracker is moved into into ExecEnv.
* Temporary query MemTrackers used to evaluate expressions during
  planning do not need to be registered globally and are owned
  directly by the RuntimeState.
* Various cleanup logic is moved around to reflect the other changes.

Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Reviewed-on: http://gerrit.cloudera.org:8080/5630
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exec/hash-table-test.cc
M be/src/exprs/expr-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/service/fe-support.cc
M be/src/service/impala-http-handler.cc
M be/src/service/query-exec-state.cc
25 files changed, 372 insertions(+), 353 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 11: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 11: Code-Review+2

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 10:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/194/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 1909: 
ExecEnv::GetInstance()->query_exec_mgr()->ReleaseQueryState(query_state_);
> feel free to add a QS::Release() convenience function if you think it's wor
I'll probably skip this for now - this is long but a one-liner still.


http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

Line 100:   static std::unique_ptr CreateQueryMemTracker(const 
TUniqueId& id,
> instead of returning a unique_ptr, wouldn't it make more sense to create th
Done


http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:

Line 59:   bool created;
> since you don't use it, best to call it dummy or something like that.
Done


http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 388:   /// Memory usage of this fragment instance, the child of 
'query_mem_tracker_'.
> "a child of"
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#10).

Change subject: IMPALA-4678: move query MemTracker into QueryState
..

IMPALA-4678: move query MemTracker into QueryState

The query MemTracker for query execution is now owned directly by
QueryState, which greatly simplifies the lifecycle of the MemTracker.
This required various other changes and enabled some simplifications.

* The coordinator's QueryState is constructed earlier before fragments
  are sent out, since we need a MemTracker at that point.
* The global query MemTracker map can be removed.
* The static request pool mem tracker is moved into into ExecEnv.
* Temporary query MemTrackers used to evaluate expressions during
  planning do not need to be registered globally and are owned
  directly by the RuntimeState.
* Various cleanup logic is moved around to reflect the other changes.

Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
---
M be/src/exec/hash-table-test.cc
M be/src/exprs/expr-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/service/fe-support.cc
M be/src/service/impala-http-handler.cc
M be/src/service/query-exec-state.cc
25 files changed, 372 insertions(+), 353 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 9:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 1909: 
ExecEnv::GetInstance()->query_exec_mgr()->ReleaseQueryState(query_state_);
feel free to add a QS::Release() convenience function if you think it's worth 
it.


http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

Line 100:   static std::unique_ptr CreateQueryMemTracker(const 
TUniqueId& id,
instead of returning a unique_ptr, wouldn't it make more sense to create this 
in the querystate's objectpool? it needs to live as long as the qs.


http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:

Line 59:   bool created;
since you don't use it, best to call it dummy or something like that.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 120: 
> Done.
yeah, we should really standardize across the entire codebase. not sure what 
the goog style guide prescribes.


http://gerrit.cloudera.org:8080/#/c/5630/7/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 388:   /// Memory usage of this fragment instance, the child of 
'query_mem_tracker_'.
"a child of"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 4:

(1 comment)

I didn't mean to push out PS8 - PS9 reverts the PS8 change.

http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 135:   std::unique_ptr query_mem_tracker_;
> MemTracker query_mem_tracker_ 
I did it this way to let us forward declare MemTracker and avoid pulling the 
MemTracker header in everywhere (since query-state.h will be included a lot of 
places). 

Also we need to pass arguments into the MemTracker constructor, which would 
force a fair bit of restructuring (could no longer use CreateQueryMemTracker(), 
etc).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#9).

Change subject: IMPALA-4678: move query MemTracker into QueryState
..

IMPALA-4678: move query MemTracker into QueryState

The query MemTracker for query execution is now owned directly by
QueryState, which greatly simplifies the lifecycle of the MemTracker.
This required various other changes and enabled some simplifications.

* The coordinator's QueryState is constructed earlier before fragments
  are sent out, since we need a MemTracker at that point.
* The global query MemTracker map can be removed.
* The static request pool mem tracker is moved into into ExecEnv.
* Temporary query MemTrackers used to evaluate expressions during
  planning do not need to be registered globally and are owned
  directly by the RuntimeState.
* Various cleanup logic is moved around to reflect the other changes.

Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
---
M be/src/exec/hash-table-test.cc
M be/src/exprs/expr-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/service/fe-support.cc
M be/src/service/impala-http-handler.cc
M be/src/service/query-exec-state.cc
25 files changed, 373 insertions(+), 352 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#8).

Change subject: IMPALA-4678: move query MemTracker into QueryState
..

IMPALA-4678: move query MemTracker into QueryState

The query MemTracker for query execution is now owned directly by
QueryState, which greatly simplifies the lifecycle of the MemTracker.
This required various other changes and enabled some simplifications:

* The coordinator's QueryState is constructed earlier before fragments
  are sent out, since we need a MemTracker at that point.
* The global query MemTracker map can be removed.
* The static request pool mem tracker is moved into into ExecEnv.
* Temporary query MemTrackers used to evaluate expressions during
  planning do not need to be registered globally and are owned
  directly by the RuntimeState.
* Various cleanup logic is moved around to reflect the other changes.

Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
---
M be/src/exec/hash-table-test.cc
M be/src/exprs/expr-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/service/fe-support.cc
M be/src/service/impala-http-handler.cc
M be/src/service/query-exec-state.cc
M be/src/udf/udf-internal.h
26 files changed, 380 insertions(+), 352 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 135:   std::unique_ptr query_mem_tracker_;
> Not sure what you intended here.
MemTracker query_mem_tracker_ 

as opposed to

unique_ptr query_mem_tracker_


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 5:

(2 comments)

Discussed a couple of the outstanding comments offline with Marcel. Made 
updates to the patch accordingly

http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 120: 
> move functions to bottom
Done.

Mostly I've seen functions at the top in backend code - this class seems to use 
the opposite convention so I guess it makes sense to be locally consistent.


Line 135:   /// Create QueryState w/ copy of query_ctx and refcnt of 0.
> inline?
Not sure what you intended here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#6).

Change subject: IMPALA-4678: move query MemTracker into QueryState
..

IMPALA-4678: move query MemTracker into QueryState

The query MemTracker for query execution is now owned directly by
QueryState, which greatly simplifies the lifecycle of the MemTracker.
This required various other changes and enabled some simplifications:

* The coordinator's QueryState is constructed earlier before fragments
  are sent out, since we need a MemTracker at that point.
* The global query MemTracker map can be removed.
* The static request pool mem tracker is moved into into ExecEnv.
* Temporary query MemTrackers used to evaluate expressions during
  planning do not need to be registered globally and are owned
  directly by the RuntimeState.
* Various cleanup logic is moved around to reflect the other changes.

Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
---
M be/src/exec/hash-table-test.cc
M be/src/exprs/expr-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/service/fe-support.cc
M be/src/service/impala-http-handler.cc
M be/src/service/query-exec-state.cc
25 files changed, 373 insertions(+), 352 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 4:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 56: #include "runtime/fragment-instance-state.h"
> there's no requirement or expectation that these are alphabetical
clang-format did this for me. The google style guide also says to do it:

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


Line 466:   
query_state_.Reset(ExecEnv::GetInstance()->query_exec_mgr()->GetOrCreateQueryState(
> there's not need for 'get' semantics here
That's true - from this callsite it always goes down the "Create" code path. We 
need the generality for other fragments though and it didn't seem worth having 
two separate implementations.


Line 485: if (coord_instance_ == nullptr) {
> leave comment that this is a startup failure scenario
Done - didn't mean to remove that.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 43: #include "scheduling/query-schedule.h"
> did you move these?
clang-format sorts includes - I added query-state.h so this happened when I ran 
clang-format on the diff. 

This is consistent with the Google style guide: 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


Line 295:   QueryState::ScopedRef query_state_;
> i'm not sure this is a good idea. we don't want this to be managed by d'tor
Done


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:

Line 193:   scoped_ptr runtime_state_;
> why?
We need to call the RuntimeState() constructor after setting up the ExecEnv, 
because it hooks up the RuntimeState's MemTracker to things in the ExecEnv.

Previously this wasn't an issue because InitMemTrackers() was split out from 
the RuntimeState constructor.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

Line 420:   /// Construct a MemTracker object for query 'id'. The query limits 
are determined based
> this is not a lookup function, make it a static function in MemTracker itse
If I rewrite it as a static function, it would end up being a static function 
with MemTrackerRegistry* as its first argument (because it needs to obtain the 
pool MemTracker from there).  Seemed cleaner just to have it as a member 
function.

This also has the advantage that one class is responsible for setting up the 
top two levels of the hierarchy correctly, rather than having responsibility 
split between two classes.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:

Line 63:   QueryState* GetOrCreateQueryState(
> are the 'get' semantics really needed?
We need it to behave in this way, since outside of the coordinator we don't 
know which fragment instance will start first. I could rename it 
CreateQueryState() - it looks like in the codebase we use both naming 
conventions for this kind of function.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

Line 69:   query_mem_tracker_->UnregisterFromParent();
> we want to get away from doing anything meaningful in the d'tors.
It's also called TearDown() in Coordinator, to add another option. I called it 
ReleaseResources() to be consistent with RuntimeState.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 75: /// Releases the previous query state (if it was non-NULL) and 
replaces it with
> is this really needed, rather than just creating a new one?
I ended up removing this, since I removed the use case in the coordinator.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS4, Line 380: this
> remove
Done


Line 389:   void InitMemTrackers();
> move above comment
This was leftover from an earlier iteration -it's not needed.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/test-env.h
File be/src/runtime/test-env.h:

Line 48:   const TQueryOptions* query_options, QueryState** query_state,
> superfluous; you can get the qs from the runtimestate.
Done


Line 53:   void TearDownStates();
> or simply TearDown()?
I wanted the name to reflect that it didn't tear down TestEnv - just the things 
owned by TestEnv that were created since the last TearDownStates() call.

Renamed to TearDownQueries() - that may be clearer about what it's meant to 
simulate.


Line 83:   std::vector 
fragment_instance_states_;
> these should be owned by the querystates
I changed this to just stick them in the QueryState object pool instead. We 
don't need this vector for anything.


Line 86:   

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#5).

Change subject: IMPALA-4678: move query MemTracker into QueryState
..

IMPALA-4678: move query MemTracker into QueryState

The query MemTracker for query execution is now owned directly by
QueryState, which greatly simplifies the lifecycle of the MemTracker.
This required various other changes and enabled some simplifications:

* The coordinator's QueryState is constructed earlier before fragments
  are sent out, since we need a MemTracker at that point.
* The global query MemTracker map can be removed.
* The static request pool mem tracker is moved into into ExecEnv.
* Temporary query MemTrackers used to evaluate expressions during
  planning do not need to be registered globally and are owned
  directly by the RuntimeState.
* Various cleanup logic is moved around to reflect the other changes.

Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
---
M be/src/exec/hash-table-test.cc
M be/src/exprs/expr-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/service/fe-support.cc
M be/src/service/impala-http-handler.cc
M be/src/service/query-exec-state.cc
25 files changed, 363 insertions(+), 351 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker