impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Date Sat, 03 Dec 2016 02:12:14 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-4014: Introduce query-wide execution state.
......................................................................


Patch Set 6:

(11 comments)

Some more comments about the refcounting, many in response to yours.

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

PS6, Line 509: qs = nullptr;
superfluous, since it is never read again.


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

PS4, Line 86: // start new thread to execute instance
            :   Thread::Run("query-exec-mgr",
            :       Substitute("exec-fragment-instance-$0", PrintId(
> i felt both approaches are equally valid.
Having some fragment instance management logic here confuses the responsibilities of this
class and of QueryState (which I would expect to control the lifecycle of the fragment instance
as well as contain its metadata). There naturally seems to be a hierarchy of responsibilities:
QueryExecMgr manages QueryStates, QueryStates manage FragmentInstances. That encapsulation
makes the code easier to reason about.


PS4, Line 115: // always decrement refcount
> i thought about this, but didn't see it as a bug.
Maybe it's not a bug as implemented, but it's counter-intuitive that there could be more than
one QS over time (it's tripped two reviewers up now). It is a simpler invariant that there
is one QES over the lifetime of the query. I think there are bugs waiting to happen. For example:
we add aggregated state / metrics to the QES and then wonder why they get reset over time.


PS4, Line 147: if (it == qs_map_.end()) return;
> after decreasing the refcnt to 0 in l138 someone else could have increased 
I think this is again unnecessarily confusing. Why not have the semantics such that:

  refcount == 0 -> no more references may be taken && caller to RQS() that sets
count == 0 performs (or schedules) the GC.

If we stipulate that a query is live for as long as its fragments are running, then having
the QES start out with refcount == 1 (like you already do) is compatible with the above invariant,
and much easier to reason about.


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

PS6, Line 76: qs->refcnt_.Load()
beware that this can change between line 72 and now. Just store the result of Add() and log
that.


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

PS4, Line 58: 
> i'll change this comment to fit the existing behavior.
Elsewhere you say that you're happy with a QS getting recreated if the original gets GCed.
I think that allowing refcount == 0 as a valid live state complicates the state machine for
a refcounted object. For now, if the refcount hits 0, it's not deterministic whether or not
a QS will get GC'ed. It's more understandable to make that relationship unambiguous, and I've
expanded on that in other comments.

Who are the clients that you expect to do a second registration? If you're concerned about
the case where a fragment completes before another fragment arrives, that seems like an edge
case that's not worth adapting the semantics for (given that that case won't be possible once
batching happens). Eventually I would expect registration to happen once per query, called
from one path only (the batched RPC handler).


http://gerrit.cloudera.org:8080/#/c/4418/6/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

PS6, Line 89: // Helper function to translate between Beeswax and HiveServer2 type
            : static TOperationState::type QueryStateToTOperationState(
            :     const beeswax::QueryState::type& query_state) {
            :   switch (query_state) {
            :     case beeswax::QueryState::CREATED: return TOperationState::INITIALIZED_STATE;
            :     case beeswax::QueryState::RUNNING: return TOperationState::RUNNING_STATE;
            :     case beeswax::QueryState::FINISHED: return TOperationState::FINISHED_STATE;
            :     case beeswax::QueryState::EXCEPTION: return TOperationState::ERROR_STATE;
            :     default: return TOperationState::UKNOWN_STATE;
            :   }
            : }
While you're here, put in anonymous namespace.


http://gerrit.cloudera.org:8080/#/c/4418/6/be/src/util/thread.cc
File be/src/util/thread.cc:

PS6, Line 305: //ThreadMgr* thread_mgr = ExecEnv::GetInstance()->thread_mgr();
?


PS6, Line 313: //thread_mgr_ref->AddThread(this_thread::get_id(), name, category, system_tid);
?


http://gerrit.cloudera.org:8080/#/c/4418/6/be/src/util/thread.h
File be/src/util/thread.h:

PS6, Line 146: /// Starts a detached thread running 'functor' and returns immediately.
             :   static void StartDetachedThread(con
What's a detached thread? Why doesn't this method register threads so that we can monitor
them?


PS6, Line 172: SuperviseDetachedThread
Needs a comment. At this moment I don't see why this exists.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message