impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState
Date Wed, 11 Jan 2017 23:42:46 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message