[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: Line 101: /// referenced as a shared_ptr to allow asynchronous calls to CancelPlanFragment() > they are, and i have a patch out (3988) that embeds the instance index into Yes, that will make it much simpler to extract the index. However, I still think that a scenario such as the following could happen: If there are 6 fragment instances for a query, frag_inst#1 -> node1 frag_inst#6 -> node1 frag_inst#2 -> node2 frag_inst#3 -> node2 rest goes to node3 So the FragmentExecState vector in node1 will have a gap between index 1 and index 6. Similarly gaps will exist for node2 and node 3. I verified this by looking at some minicluster logs and the fragment instance IDs are not contiguous per backend. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Marcel Kornacker has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: Line 101: /// referenced as a shared_ptr to allow asynchronous calls to CancelPlanFragment() > Having shared_ptrs are how it was done before. I've changed it to use raw p they are, and i have a patch out (3988) that embeds the instance index into the instance id in a way that makes it easy to extract the index, given just the instance id (you won't need the query id). the index will always be 0..#instances-1. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has uploaded a new patch set (#6). Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. PREVIEW: IMPALA-2550 Introduce query-wide execution context. This patch is a header preview of a query wide execution state for a backend. Changes in design are as follows: FragmentMgr -> QueryExecMgr The QueryExecMgr now receives incoming fragments, creates a new QueryExecState if it is the first fragment to arrive for that query, and passes this fragment along to the QueryExecState. This class is responsible for cleaning up the QueryExecState. QueryExecState (new): This is the query wide state for the backend. It is initialized by the first fragment to arrive to the QueryExecMgr. This class is now responsible for creating FragmentExecStates and executing them. Its life is protected by a ref counting mechanism and it is scheduled for destruction once the ref count reaches zero. Once scheduled for destruction, a thread in the QueryExecMgr will destroy the QueryExecState. Every user of the QueryExecState must access it within the scope of ScopedQueryExecStateRef which guarantees ref count incrementing and decrementing at entry/exit of the scope. QueryExecState (old) -> ClientRequestExecState: This is just a class name change. P.S: This only shows what the changes would look like and as of yet still does not compile. Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c --- M be/src/runtime/CMakeLists.txt M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h R be/src/runtime/fragment-instance-exec-state.cc A be/src/runtime/fragment-instance-exec-state.h R be/src/runtime/fragment-instance-executor.cc R be/src/runtime/fragment-instance-executor.h A be/src/runtime/query-exec-state.cc A be/src/runtime/query-exec-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/CMakeLists.txt R be/src/service/client-request-exec-state.cc R be/src/service/client-request-exec-state.h D be/src/service/fragment-exec-state.h D be/src/service/fragment-mgr.cc D be/src/service/fragment-mgr.h M be/src/service/impala-internal-service.h A be/src/service/query-exec-mgr.cc A be/src/service/query-exec-mgr.h M common/thrift/ImpalaInternalService.thrift 21 files changed, 847 insertions(+), 471 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/6 -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 5:
(33 comments)
http://gerrit.cloudera.org:8080/#/c/3817/5//COMMIT_MSG
Commit Message:
PS5, Line 15: recieves
> receives
Done
PS5, Line 17:
> A comma would be helpful here.
Done
PS5, Line 24: and is
> "and it is" ?
Done
PS5, Line 24: which
> This seems like an incomplete thought. Perhaps the "which" just needs to be
Done
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:
Line 77: TPlanFragmentInstanceCtx fragment_instance_ctx_;
> remove, unless there's something to do
Done
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:
Line 32: /// Execution state of a single plan fragment.
> what differentiates this from RuntimeState. ie, are these really two separa
The RuntimeState contains mostly the members populated at runtime like the
QueryMemTracker, runtime profile counters, block managers, etc.
The FragmentExecState mostly has execution state associated with it and sets up
the PlanFragmentExecutor.
If we were to merge them, that should be done as a separate patch as it would
be quite a big change. However, I feel that would make the class pretty bloated.
PS5, Line 33: FragmentExecState
> Are we going to rename this to make it clear that it's instance-specific st
Yes, I've renamed it.
Line 40: boost::mem_fn(&FragmentExecState::ReportStatusCb),
> lambda
Done
Line 42: client_cache_(exec_env->impalad_client_cache()),
exec_params_(params) {
> break into two lines, all other members are on separate lines
Done
Line 80: // TODO: make pointer to shared per-query state
> what does this todo mean?
I forgot to remove that. Removed it now.
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:
Line 64: class PlanFragmentExecutor {
> needs new name. FragmentInstanceExecutor?
Done
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 55: ObjectPool* obj_pool() const { return obj_pool_.get(); }
> .get()?
Oops, removed it.
Line 79: QueryExecMgr* query_exec_mgr_;
> presumably "not owned"
Done
Line 85: ObjectPool* obj_pool_;
> owner?
Done
Line 101: /// referenced as a shared_ptr to allow asynchronous calls to
CancelPlanFragment()
> why do we need a shared_ptr? we should get rid of implicit mem mgmt with sh
Having shared_ptrs are how it was done before. I've changed it to use raw
pointers now though with an explicit mem mgmt model.
Are fragment instance numbers guaranteed to be consecutive in a backend? If
not, we will have gaps in the vector/array.
Line 108: std::shared_ptr GetFragmentExecState(
> remove shared_ptr or unique_ptr since we're now switching to an explicit me
Done
Line 115: void FragmentInstanceExecThread(const TUniqueId&
fragment_instance_id);
> why not just ExecInstance?
Done
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/client-request-exec-state.h
File be/src/service/client-request-exec-state.h:
Line 44: /// Execution state of a query. This captures everything necessary
> clarify comment. this isn't just the "execution state".
Done
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
PS4, Line 80: eId& fragment_instance_id);
> we can also encode the instance id as query id + instance offset/instance n
Yes I saw the patch for IMPALA-3988. This function has been removed in any case
in the next patch set.
Line 91: /// The caller should always check if obj->is_valid_ref() is 'true'
before using it.
> please find a more concise name (maybe take inspiration from 'lock_guard'?)
Renamed it to QESGuard. Done.
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
Line 40: /// offer itself up for destruction when it is no longer being used
and the
> 'no longer being used' is vague. mention (and make concrete in code) the re
Done
Line 59: /// no-op (because the fragment is unregistered).
> you're also introducing terminology 'registered' and 'unregistered'. what a
The registered and unregistered semantics have already been in use.
'Registered' means it has been successfully created and put in the map.
'Unregistered' means that it cannot be found on the map any longer.
Now with the introduction of ref counts, it is the same as refcount>0 and
refcount==0, because if it exists on the map, the refcount>0 and once the
refcount==0, it's immediately removed from the map.
Line 60: Status RegisterFragmentInstanceWithQuery(const
TExecPlanFragmentParams& params);
> why doesn't this return the qes? you'd typ
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Dan Hecht has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/fragment-exec-state.h File be/src/runtime/fragment-exec-state.h: PS5, Line 33: FragmentExecState Are we going to rename this to make it clear that it's instance-specific state? -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Marcel Kornacker has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 5:
(25 comments)
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:
Line 32: /// Execution state of a single plan fragment.
what differentiates this from RuntimeState. ie, are these really two separate
abstractions (and if so, what are the differentiating characteristics)?
Line 40: boost::mem_fn(&FragmentExecState::ReportStatusCb),
lambda
Line 42: client_cache_(exec_env->impalad_client_cache()),
exec_params_(params) {
break into two lines, all other members are on separate lines
Line 80: // TODO: make pointer to shared per-query state
what does this todo mean?
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:
Line 64: class PlanFragmentExecutor {
needs new name. FragmentInstanceExecutor?
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 56: const DescriptorTbl& desc_tbl() const { return *desc_tbl_; }
> 'Accept' was just a way to say that it was getting it from the QueryExecMgr
my point was: don't use superfluous ('incoming': all fragment instances are
incoming) words or words that convey no meaning ('accept'?) in names, they
don't help the reader of your code.
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 55: ObjectPool* obj_pool() const { return obj_pool_.get(); }
.get()?
Line 79: QueryExecMgr* query_exec_mgr_;
presumably "not owned"
Line 85: ObjectPool* obj_pool_;
owner?
Line 101: /// referenced as a shared_ptr to allow asynchronous calls to
CancelPlanFragment()
why do we need a shared_ptr? we should get rid of implicit mem mgmt with
shared_ptrs. anyone with an active FragmentExecState should also be included in
the ref count.
also, this doesn't need to be a map, you can use an array/vector instead (and
use instance_id - query_id as the index.
Line 108: std::shared_ptr GetFragmentExecState(
remove shared_ptr or unique_ptr since we're now switching to an explicit mem
mgmt model
Line 115: void FragmentInstanceExecThread(const TUniqueId&
fragment_instance_id);
why not just ExecInstance?
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/client-request-exec-state.h
File be/src/service/client-request-exec-state.h:
Line 44: /// Execution state of a query. This captures everything necessary
clarify comment. this isn't just the "execution state".
http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
Line 40: /// offer itself up for destruction when it is no longer being used
and the
'no longer being used' is vague. mention (and make concrete in code) the
reference counting mechanism.
Line 59: /// no-op (because the fragment is unregistered).
you're also introducing terminology 'registered' and 'unregistered'. what are
the semantics of that? is this distinct from live/refcount > 0 and
not-live/refcount == 0?
Line 60: Status RegisterFragmentInstanceWithQuery(const
TExecPlanFragmentParams& params);
why doesn't this return the qes? you'd typically want to access that right
after this call.
Line 62: /// Return a pointer to the QueryExecState if one can be found for
the given query id
describe the effect on the reference count (a function comment should describe
the externally observable behavior of a function, and a change in the refcount
is one of the main side effects of calling this function).
this is also missing elsewhere.
Line 69: /// Decrements query_exec_state->num_active_references and schedules
it for destruction
reference counting is a central concept here, it needs to be described in a
class comment.
also, i couldn't find the reference count anywhere so far.
also, why isn't this a member function of qes itself?
Line 75: Status GetQueryId(const TUniqueId& fragment_instance_id, TUniqueId*
query_id);
why do we need this?
Line 87: /// This class c'tor takes a query_id and guarantees the caller one
of two things:
i think something like a lock_guard might be more appropriate - it guarantees
unlocking/decreasing the refcount, but it doesn't mediate access. what you have
will get unwieldy very quickly.
Line 97: /// TODO: Is this really necessary? The downside is that this class
needs to duplicate
i don't understand the purpose of preventing access to the qes.
transfer ownership where?
Line 99: class ScopedQueryExecStateRef {
why is this local to qem?
Line 124: QueryExecMgr* query_exec_mgr_;
why do you need this?
Line 126: const TUniqueId query_id_;
do you need this if query_exec_state_ is null?
Line 137:
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
David Knupp has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/3817/5//COMMIT_MSG Commit Message: PS5, Line 15: recieves receives PS5, Line 17: A comma would be helpful here. PS5, Line 24: and is "and it is" ? PS5, Line 24: which This seems like an incomplete thought. Perhaps the "which" just needs to be removed? -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Marcel Kornacker has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. Patch Set 5: (1 comment) please do a pass to disambiguate fragments and their instances (e.g., FragmentExecState). http://gerrit.cloudera.org:8080/#/c/3817/5/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 499: spaces -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Marcel Kornacker has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
PS4, Line 418: // TODO: Should we create a QueryExecState for the coordinator
fragment?
> I thought the coordinator does not have a QEM (or previously a FragmentMgr)
for the sake of keeping this patch small, it's probably a good idea to leave
out the restructuring of the coordinator (plus, this has lots of conflicts with
my next set of mt-related changes)
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:
Line 77: // TODO: make pointer to shared per-query state
remove, unless there's something to do
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
PS4, Line 80: GetQueryExecStateFromFragmentId
> As we spoke everything coming in to a backend specifies only the fragment_i
we can also encode the instance id as query id + instance offset/instance
number. that way we can build static arrays, indexed by instance number, that
live in the qes.
Line 91: class ScopedQueryExecStateRef {
> Done
please find a more concise name (maybe take inspiration from 'lock_guard'?)
--
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Sailesh Mukil
Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 5:
(30 comments)
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
PS4, Line 418: // TODO: Should we create a QueryExecState for the coordinator
fragment?
> yes, otherwise you wind up with special-case logic to handle the paths wher
I thought the coordinator does not have a QEM (or previously a FragmentMgr).
Because we never create a FragmentExecState for the coordinator fragment and
just start off with a PlanFragmentExecutor directly. And the coordinator code
ends up doing what a FragmentExecState would have done for a fragment otherwise
(like Prepare(), Exec(), Close(), etc.)
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/query-exec-state.cc
File be/src/runtime/query-exec-state.cc:
PS4, Line 71: lock_guard l(fragment_exec_state_map_lock_);
> This is confusing: presumably this exists to ensure that *this has a lifeti
That makes sense. I previously wanted only a single interface to
increase/decrease the ref counter, but I see the problem with it now.
I've made FindOrInsertQES() increase the ref count.
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> Fix license headers here and elsewhere.
Done
Line 36: /// Backend execution state of a query, which is shared between all
fragment instances
> No need to talk about what owners of this class might do with it.
Done
PS4, Line 41: or f
> nit: use nullptr now.
Done
PS4, Line 84:
> Why shared?
That was a mistake. Changed it to a raw pointer.
PS4, Line 101: rence
> std
As we spoke, I'll make sure that the decided on patch compiles with 'boost' and
change it to 'std' in the end (as changing to 'std' causes some compilation
issues I don't really understand now).
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
Line 274: /// A pointer to the query wide execution state which holds all the
shared state between
> comment what this is.
Done
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/client-request-exec-state.cc
File be/src/service/client-request-exec-state.cc:
Line 14:
> add new line before this one
Done
Line 570: wait_thread_.reset(new Thread("query-exec-state", "wait-thread",
> long line
Done
Line 677:
> long line
Done
Line 829: }
> long line
Done. Just FYI, there's quite some more to fix in this file, but haven't done
it yet as it's not relevant to the core patch. Will do it once the header is
decided on.
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/query-exec-mgr.cc
File be/src/service/query-exec-mgr.cc:
PS4, Line 37:
> avoid bind, prefer lambdas
I've made a note of this, I will change it once I can get past all the
compilation issues after the header has been decided on.
PS4, Line 48: << " fragment instance#="
: <<
exec_params.fragment_instance_ctx.instance_state_idx;
:
: // Remote fragments must always have a sink. Remove when
IMPALA-2905 is resolved.
: DCHECK(exec_params.fragment_ctx.fragment.__isset.output_sink);
:
: // Create or retrieve the QueryExecState for this query id.
: QueryExecState* query_exec_state =
FindOrInsertQueryExecState(exec_params.query_ctx);
: DCHECK(query_exec_state != NULL);
:
: // This is a no-op if another fragment of the same query
already called Init().
: query_exec_state->Init();
:
> this logic looks like it should live inside RegisterFragmentInstance
Done
PS4, Line 65: if (!register_status.ok()) {
: ReleaseQueryExecState(query_exec_state);
: return register_status;
: }
: return Status::OK();
: }
> Why not change the interface to the following:
I thought of that but GetQES() is used wherever we want to get the QES for a
query.
So, this could result in a case where the QES is destroyed in a backend (after
that backend completes its execution for that query), and then a late
CancelPlanFragment() arrives and calls GetQES() which will end up setting up
the QES for that already completed query (w.r.t that backend) again.
I've made a change so that FindOrInsertQES() also increases the ref count, and
calls ReleaseQES() on a failure.
PS4, Line 103: lock_guard l(query_exec_sta
> ++
Done
PS4, Line 107: unique_ptr query_exec_state(new
QueryExecState(this, query_ctx));
: i = query_exec_state_map_.insert(
: make_pair(query_id, std::move(query_exec_state)));
: }
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has uploaded a new patch set (#5). Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. PREVIEW: IMPALA-2550 Introduce query-wide execution context. This patch is a header preview of a query wide execution state for a backend. Changes in design are as follows: FragmentMgr -> QueryExecMgr The QueryExecMgr now recieves incoming fragments, creates a new QueryExecState if it is the first fragment to arrive for that query and passes this fragment along to the QueryExecState. This class is responsible for cleaning up the QueryExecState. QueryExecState (new): This is the query wide state for the backend. It is initialized by the first fragment to arrive to the QueryExecMgr. This class is now responsible for creating FragmentExecStates and executing them. Its life is protected by a ref counting mechanism which and is scheduled for destruction once the ref count reaches zero. Once scheduled for destruction, a thread in the QueryExecMgr will destroy the QueryExecState. Every user of the QueryExecState must access it within the scope of ScopedQueryExecStateRef which guarantees ref count incrementing and decrementing at entry/exit of the scope. QueryExecState (old) -> ClientRequestExecState: This is just a class name change. P.S: This only shows what the changes would look like and as of yet still does not compile. Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c --- M be/src/runtime/CMakeLists.txt M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h R be/src/runtime/fragment-exec-state.cc R be/src/runtime/fragment-exec-state.h M be/src/runtime/plan-fragment-executor.cc M be/src/runtime/plan-fragment-executor.h A be/src/runtime/query-exec-state.cc A be/src/runtime/query-exec-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/CMakeLists.txt R be/src/service/client-request-exec-state.cc R be/src/service/client-request-exec-state.h D be/src/service/fragment-mgr.cc D be/src/service/fragment-mgr.h M be/src/service/impala-internal-service.h A be/src/service/query-exec-mgr.cc A be/src/service/query-exec-mgr.h M common/thrift/ImpalaInternalService.thrift 20 files changed, 718 insertions(+), 351 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/5 -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Henry Robinson has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 4:
(30 comments)
This is my first pass, and so doesn't capture all the style, comment etc
issues. I'm first focusing on whether the ScopedQueryExecStateRef works as
intended, and whether the lookup-by-fragment ID is required.
I'm not yet convinced that this is coming out more cleanly than a shared_ptr
with a custom deleter (which is effectively what we are doing here).
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
PS4, Line 418: // TODO: Should we create a QueryExecState for the coordinator
fragment?
yes, otherwise you wind up with special-case logic to handle the paths where
the QES is null.
Why does this QES not go in the local QEM?
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/query-exec-state.cc
File be/src/runtime/query-exec-state.cc:
PS4, Line 71: QueryExecMgr::ScopedQueryExecStateRef s(query_exec_mgr,
fragment_instance_id);
This is confusing: presumably this exists to ensure that *this has a lifetime
at least as long as this method. But if there's a danger that *this will be
destroyed during this method, that could happen before this line ever gets
executed. So this won't have a reference to anything (and will actually crash).
Why not have a fragment exec state add a reference to the QES that is
decremented when the FES completes? Then the reference is guaranteed to be > 0
after line 49 above, and you don't need to take the reference here.
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 1: // Copyright 2016 Cloudera Inc.
Fix license headers here and elsewhere.
Line 36: /// The lifetime of this class is maintained by QueryExecMgr.
No need to talk about what owners of this class might do with it.
PS4, Line 41: NULL
nit: use nullptr now.
PS4, Line 84: std::shared_ptr
Why shared?
PS4, Line 101: boost
std
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
Line 274: QueryExecState* query_exec_state_; // not owned
comment what this is.
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/client-request-exec-state.cc
File be/src/service/client-request-exec-state.cc:
Line 14: #include "service/request-exec-state.h"
add new line before this one
Line 570: "query-exec-state", "wait-thread",
&ImpalaServer::ClientRequestExecState::Wait, this));
long line
Line 677: void
ImpalaServer::ClientRequestExecState::UpdateNonErrorQueryState(QueryState::type
query_state) {
long line
Line 829: Status ImpalaServer::ClientRequestExecState::GetRowValue(TupleRow*
row, vector* result,
long line
http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/query-exec-mgr.cc
File be/src/service/query-exec-mgr.cc:
PS4, Line 37: bind(mem_fn(&QueryExecMgr::DestroyQueryExecState), this,
_1, _2)
avoid bind, prefer lambdas
PS4, Line 48: const TUniqueId& query_id = exec_params.query_ctx.query_id;
: // Preparing and opening the fragment creates a thread and
consumes a non-trivial
: // amount of memory. If we are already starved for memory,
cancel the fragment as
: // early as possible to avoid digging the hole deeper.
: MemTracker* process_mem_tracker =
ExecEnv::GetInstance()->process_mem_tracker();
: if (process_mem_tracker->LimitExceeded()) {
: string msg = Substitute("Instance $0 of plan fragment $1 of
query $2 could not "
: "start because the backend Impala daemon is over its
memory limit",
:
PrintId(exec_params.fragment_instance_ctx.fragment_instance_id),
: exec_params.fragment_ctx.fragment.display_name,
: PrintId(query_id));
: return process_mem_tracker->MemLimitExceeded(NULL, msg, 0);
: }
this logic looks like it should live inside RegisterFragmentInstance
PS4, Line 65: // Create or retrieve the QueryExecState for this query id.
: QueryExecState* query_exec_state =
FindOrInsertQueryExecState(exec_params.query_ctx);
: DCHECK(query_exec_state != NULL);
:
: // This is a no-op if another fragment of the same query
already called Init().
: query_exec_state->Init();
Why not change the interface to the following:
// Creates a QES if one doesn't exist.
qes = QEM->GetQueryExecState(query_id);
qes->RegisterFragment(fragment_id);
Then the Init() is only ever done in one place (GetQES()), and you directly
interact with the QES itself.
PS4, Line 103: i->second->num_current_references_++;
++
PS4, Line 107: void QueryExecMgr::ReleaseQueryExecState(QueryExecState*
query_exec_state) {
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 3:
(23 comments)
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:
Line 72: // TODO: make pointer to shared per-query state
> better yet: get rid of this and provide a convenience function that goes th
Done
Line 77: // TODO: make pointer to shared per-query state
> same here
This got carried over from Lars patch. I had overlooked it the last time. Do
you mean I should create a shared client_cache between all the fragments vs. a
single client cache per fragment?
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:
Line 209: request.fragment_ctx.fragment.plan,
runtime_state_->query_exec_state()->desc_tbl(),
> why go through runtime_state_?
I've changed it to just access it from its own pointer 'query_exec_state_'.
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.cc
File be/src/runtime/query-exec-state.cc:
PS3, Line 72: QueryExecMgr::ScopedQueryExecStateRef s(*query_exec_mgr, this);
> Bug: I would have to call GetQES(query_ctx_.query_id) before this line to i
This bug no longer exists as I call GetQES() inside the
ScopedQueryExecStateRef() c'tor.
However, I still feel this is somewhat suboptimal.
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 44: /// Initializes common data structures (descriptor table, ...). This
method can be
> precede w/ blank line
Done
Line 56: Status AcceptIncomingFragment(const TExecPlanFragmentParams&
exec_params);
> what does 'accept' mean? are there any non-incoming fragments? are these re
'Accept' was just a way to say that it was getting it from the QueryExecMgr. I
said 'Incoming' because it came in as a remote fragment instance.
In any case I've changed the function name to RegisterFragmentInstance().
Line 63: void PublishFilter(TPublishFilterResult& return_val,
> follow conventions for output param
Done
Line 84: const TQueryCtx& query_ctx_;
> who owns this?
It's owned by this class. Removed the const ref.
Line 91: int32_t num_active_references_;
> are there inactive references?
No, changed it to num_current_references_.
Line 97: /// referenced as a shared_ptr to allow asynchronous calls to
CancelPlanFragment()
> why do async calls require a shared_ptr?
I didn't write a lot of this code (I moved it from fragment-exec-state), but
looking at the code the shared_ptr is required because if CancelPlanFragment()
is called for a fragment and just while beginning the cancellation, if the
fragment completes executing, it will be removed from the FragmentExecStateMap
causing the FragmentExecState to go out of scope.
So accessing it as a shared_ptr ensures that it's still available for the scope
of CancelPlanFragment()
Line 99: FragmentExecStateMap;
> indentation
Done
Line 111: void FragmentThread(TUniqueId fragment_instance_id);
> const ref
Done
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:
Line 42: TUniqueId query_id = exec_params.query_ctx.query_id;
> const ref
Done
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.h
File be/src/service/fragment-mgr.h:
Line 28: /// Manages execution of individual plan fragment instances, which are
typically run as a
> as part of the header preview, please also rewrite the class comments to fu
Sorry about that. I've made the change here and elsewhere.
Line 42: : destroy_thread_("QueryExecMgr", "DestroyThread", 1, 1,
> please move function bodies into the .cc file unless needed in the .h for p
Done
Line 45: /// Receives a remote fragment and adds it to the
fragment_instance_query_id_map_.
> don't reference private state in the public interface. the function comment
Done
Line 65: /// and if the QueryExecState is not already scheduled for
destruction.
> what are the lifetime guarantees of the returned state? does the caller nee
Done
Line 70: void ReturnQueryExecState(QueryExecState* query_exec_state);
> 'release' is a more common term for this operation
Done
Line 73: /// fragment intance id. Otherwise the pointer will contain NULL.
> spelling
Done
PS3, Line 81: class ScopedQueryExecStateRef {
> we probably need both.
Done
Line 84: QueryExecMgr& query_exec_mgr, QueryExecState* query_exec_state) :
> don't use ref params
Done.
Since every reference is per fragment, I've made the second parameter
'fragment_instance_id' instead of 'query_id', so that we don't need to look up
another map to find the query_id before calling this.
Line 102: /// reference counter to track the number or currently running
fragmen
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has uploaded a new patch set (#4). Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. PREVIEW: IMPALA-2550 Introduce query-wide execution context. This patch is a header preview of a query wide execution state for a backend. Changes in design are as follows: FragmentMgr -> QueryExecMgr The QueryExecMgr now recieves incoming fragments, creates a new QueryExecState if it is the first fragment to arrive for that query and passes this fragment along to the QueryExecState. This class is responsible for cleaning up the QueryExecState. QueryExecState (new): This is the query wide state for the backend. It is initialized by the first fragment to arrive to the QueryExecMgr. This class is now responsible for creating FragmentExecStates and executing them. Its life is protected by a ref counting mechanism which and is scheduled for destruction once the ref count reaches zero. Once scheduled for destruction, a thread in the QueryExecMgr will destroy the QueryExecState. Every user of the QueryExecState must access it within the scope of ScopedQueryExecStateRef which guarantees ref count incrementing and decrementing at entry/exit of the scope. QueryExecState (old) -> ClientRequestExecState: This is just a class name change. P.S: This only shows what the changes would look like and as of yet still does not compile. Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c --- M be/src/runtime/CMakeLists.txt M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h R be/src/runtime/fragment-exec-state.cc R be/src/runtime/fragment-exec-state.h M be/src/runtime/plan-fragment-executor.cc M be/src/runtime/plan-fragment-executor.h A be/src/runtime/query-exec-state.cc A be/src/runtime/query-exec-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/CMakeLists.txt R be/src/service/client-request-exec-state.cc R be/src/service/client-request-exec-state.h D be/src/service/fragment-mgr.cc D be/src/service/fragment-mgr.h A be/src/service/query-exec-mgr.cc A be/src/service/query-exec-mgr.h 18 files changed, 652 insertions(+), 323 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/4 -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Marcel Kornacker has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 3:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:
Line 72: // TODO: make pointer to shared per-query state
better yet: get rid of this and provide a convenience function that goes
through query_exec_state_
you can do this now, this shouldn't be many more lines of code than what you're
removing. (if a todo is trivial to resolve, it's good practice to do that right
away.)
Line 77: // TODO: make pointer to shared per-query state
same here
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:
Line 209: request.fragment_ctx.fragment.plan,
runtime_state_->query_exec_state()->desc_tbl(),
why go through runtime_state_?
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 44: /// Initializes common data structures (descriptor table, ...). This
method can be
precede w/ blank line
Line 56: Status AcceptIncomingFragment(const TExecPlanFragmentParams&
exec_params);
what does 'accept' mean? are there any non-incoming fragments? are these really
fragment instances?
Line 63: void PublishFilter(TPublishFilterResult& return_val,
follow conventions for output param
Line 84: const TQueryCtx& query_ctx_;
who owns this?
Line 91: int32_t num_active_references_;
are there inactive references?
Line 97: /// referenced as a shared_ptr to allow asynchronous calls to
CancelPlanFragment()
why do async calls require a shared_ptr?
Line 99: FragmentExecStateMap;
indentation
Line 111: void FragmentThread(TUniqueId fragment_instance_id);
const ref
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:
Line 42: TUniqueId query_id = exec_params.query_ctx.query_id;
const ref
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.h
File be/src/service/fragment-mgr.h:
Line 42: : destroy_thread_("QueryExecMgr", "DestroyThread", 1, 1,
please move function bodies into the .cc file unless needed in the .h for
performance. this reduces the rebuild footprint of a code change.
Line 45: /// Receives a remote fragment and adds it to the
fragment_instance_query_id_map_.
don't reference private state in the public interface. the function comment
should completely describe the observable behavior, but not the implementation.
distinguish between fragments and their instances. all execution requires an
instance.
Line 65: /// and if the QueryExecState is not already scheduled for
destruction.
what are the lifetime guarantees of the returned state? does the caller need to
release it?
Line 70: void ReturnQueryExecState(QueryExecState* query_exec_state);
'release' is a more common term for this operation
Line 73: /// fragment intance id. Otherwise the pointer will contain NULL.
spelling
same question about what is required of the caller
PS3, Line 81: class ScopedQueryExecStateRef {
> I don't know if this is a good way, or if we should have every caller call
we probably need both.
having this is the preferable way where the reference stays within a single
block.
Line 84: QueryExecMgr& query_exec_mgr, QueryExecState* query_exec_state) :
don't use ref params
given that your GetQueryExecState functions don't return Status, why not embed
the call in the c'tor; ie:
Scoped...(QueryExecMgr* mgr, const TUniqueId& query_id);
Line 102: /// reference counter to track the number or currently running
fragment instances. When
'number of'? also, why does the ref counter only track the currently running
instances? i'd assume that every accessor will need to increase the ref counter.
--
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Sailesh Mukil
Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Marcel Kornacker has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.h
File be/src/service/fragment-mgr.h:
Line 28: /// Manages execution of individual plan fragment instances, which are
typically run as a
as part of the header preview, please also rewrite the class comments to fully
reflect the new abstraction. this is necessary to spell out the intention and
also makes it easier to compare the intention with the interface.
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/request-exec-state.h
File be/src/service/request-exec-state.h:
Line 55: class ImpalaServer::RequestExecState {
request is very generic. this is specifically for client requests, so
ClientRequestExecState would be better.
--
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Sailesh Mukil
Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.cc File be/src/runtime/query-exec-state.cc: PS3, Line 72: QueryExecMgr::ScopedQueryExecStateRef s(*query_exec_mgr, this); Bug: I would have to call GetQES(query_ctx_.query_id) before this line to increment the ref count (as the lock governing atomic increase/decrease of the ref count is the QueryExecMgr wide query_exec_state_map_lock_ itself; which means only one thread across all queries can get a single QES at a given time). This seems really messy and can be completely avoided by using shared pointers. With shared pointers, during object destruction, I could hold the object level lock while removing the QES from the query_exec_state_map_. Now, since the map contains a unique_ptr, I cannot hold an object level lock while removing the QES object from the map. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has posted comments on this change.
Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
PS3, Line 87: num_active_fragment_instances_
This is not really used right now. May remove it if there doesn't seem to be
any use for it.
http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.h
File be/src/service/fragment-mgr.h:
PS3, Line 81: class ScopedQueryExecStateRef {
I don't know if this is a good way, or if we should have every caller call
GetQES() and ReturnQES() explicitly. Open for suggestions.
--
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Sailesh Mukil
Gerrit-HasComments: Yes
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has uploaded a new patch set (#3). Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. PREVIEW: IMPALA-2550 Introduce query-wide execution context. This patch is a header preview of a query wide execution state for a backend. Changes in design are as follows: FragmentMgr -> QueryExecMgr The QueryExecMgr now recieves incoming fragments, creates a new QueryExecState if it is the first fragment to arrive for that query and passes this fragment along to the QueryExecState. This class is responsible for cleaning up the QueryExecState. QueryExecState (new): This is the query wide state for the backend. It is initialized by the first fragment to arrive to the QueryExecMgr. This class is now responsible for creating FragmentExecStates and executing them. Its life is protected by a ref counting mechanism which and is scheduled for destruction once the ref count reaches zero. Once scheduled for destruction, a thread in the QueryExecMgr will destroy the QueryExecState. Every user of the QueryExecState must access it within the scope of ScopedQueryExecStateRef which guarantees ref count incrementing and decrementing at entry/exit of the scope. QueryExecState (old) -> RequestExecState: This is just a class name change. P.S: This only shows what the changes would look like and as of yet still does not compile. Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c --- M be/src/runtime/CMakeLists.txt M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h R be/src/runtime/fragment-exec-state.cc R be/src/runtime/fragment-exec-state.h M be/src/runtime/plan-fragment-executor.cc M be/src/runtime/plan-fragment-executor.h A be/src/runtime/query-exec-state.cc A be/src/runtime/query-exec-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/CMakeLists.txt M be/src/service/fragment-mgr.cc M be/src/service/fragment-mgr.h R be/src/service/request-exec-state.cc R be/src/service/request-exec-state.h 16 files changed, 499 insertions(+), 180 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/3 -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. Patch Set 2: Other than the headers, I've also coded up some of the important functions in QueryExecMgr and QueryExecState. There's probably a few syntax errors around, which I will get to fixing later on. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Sailesh Mukil has uploaded a new patch set (#2). Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. .. PREVIEW: IMPALA-2550 Introduce query-wide execution context. This patch is a header preview of a query wide execution state for a backend. Changes in design are as follows: FragmentMgr -> QueryExecMgr The QueryExecMgr now recieves incoming fragments, creates a new QueryExecState if it is the first fragment to arrive for that query and passes this fragment along to the QueryExecState. This class is responsible for cleaning up the QueryExecState. QueryExecState (new): This is the query wide state for the backend. It is initialized by the first fragment to arrive to the QueryExecMgr. This class is now responsible for creating FragmentExecStates and executing them. Its life is protected by a ref counting mechanism which and is scheduled for destruction once the ref count reaches zero. Once scheduled for destruction, a thread in the QueryExecMgr will destroy the QueryExecState. Every user of the QueryExecState must access it within the scope of ScopedQueryExecStateRef which guarantees ref count incrementing and decrementing at entry/exit of the scope. QueryExecState (old) -> RequestExecState: This is just a class name change. P.S: This only shows what the changes would look like and as of yet still does not compile. Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c --- M be/src/runtime/CMakeLists.txt M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h R be/src/runtime/fragment-exec-state.cc R be/src/runtime/fragment-exec-state.h M be/src/runtime/plan-fragment-executor.cc M be/src/runtime/plan-fragment-executor.h A be/src/runtime/query-exec-state.cc A be/src/runtime/query-exec-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/CMakeLists.txt M be/src/service/fragment-mgr.cc M be/src/service/fragment-mgr.h R be/src/service/request-exec-state.cc R be/src/service/request-exec-state.h 16 files changed, 497 insertions(+), 180 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/2 -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil
