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/01/10 20:02:58 UTC
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5630
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(+), 356 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/5630/4
--
To view, visit http://gerrit.cloudera.org:8080/5630
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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<MemTracker> 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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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<RuntimeState> 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<std::unique_ptr<FragmentInstanceState>> 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: std::vector<std::unique_ptr<RuntimeState>> runtime_states_;
> these should be owned by the instancestates
I also added this to the QueryState object pool. Simulating the normal ownership of FragmentInstanceState->PlanFragmentExecutor->RuntimeState would be pretty invasive.
http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
Line 100: RuntimeState state(query_ctx, ExecEnv::GetInstance(), "fe-eval-exprs");
> does the pool name signify anything? if not, leave at ""?
It shows up in "Memory limit exceeded" output - I just gave it a name so it would be easier to figure out where the memory was coming from.
Added a comment to briefly explain.
http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:
Line 1055: DCHECK(coord_->query_state()->query_mem_tracker() != NULL);
> you could also add a Coordinator::query_mem_tracker() convenience function
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:
Line 1055: DCHECK(coord_->query_state()->query_mem_tracker() != NULL);
you could also add a Coordinator::query_mem_tracker() convenience function
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4678: move query MemTracker into QueryState
......................................................................
Patch Set 10: Code-Review+2
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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<MemTracker> 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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
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 <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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<MemTracker> query_mem_tracker_;
> Not sure what you intended here.
MemTracker query_mem_tracker_
as opposed to
unique_ptr<MemTracker> 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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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<MemTracker> 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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4678: move query MemTracker into QueryState
......................................................................
Patch Set 4:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
Line 379
nice. i could never remember all the places where these things got created and destroyed.
Line 56: #include "runtime/fragment-instance-state.h"
there's no requirement or expectation that these are alphabetical
Line 466: query_state_.Reset(ExecEnv::GetInstance()->query_exec_mgr()->GetOrCreateQueryState(
there's not need for 'get' semantics here
Line 485: if (coord_instance_ == nullptr) {
leave comment that this is a startup failure scenario
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?
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'tors, and while it gets explicitly reset in TearDown(), this could potentially get overlooked.
since the intention is to manage lifetimes explicitly, i think this should be a simple QueryState*
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<RuntimeState> runtime_state_;
why?
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 itself (and rename this class to PoolMemTrackerRegistry, it doesn't track query trackers anyway).
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?
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.
i left out a ReleaseResources() (or should we call it Close()?) call when i introduced the qs class, but it looks like that's needed now.
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?
Line 120: void InitMemTrackers(const std::string& pool);
move functions to bottom
Line 135: std::unique_ptr<MemTracker> query_mem_tracker_;
inline?
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
Line 389: void InitMemTrackers();
move above comment
leave comment when this needs to get called
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.
Line 53: void TearDownStates();
or simply TearDown()?
Line 83: std::vector<std::unique_ptr<FragmentInstanceState>> fragment_instance_states_;
these should be owned by the querystates
Line 86: std::vector<std::unique_ptr<RuntimeState>> runtime_states_;
these should be owned by the instancestates
http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
Line 100: RuntimeState state(query_ctx, ExecEnv::GetInstance(), "fe-eval-exprs");
does the pool name signify anything? if not, leave at ""?
--
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 <ta...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes