[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide
execution state.
..
Patch Set 2:
(29 comments)
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
PS1, Line 68: /// on whether the fragment instance has a sink) indicated that
execution is finished.
: ~FInstanceState() { }
> If Cancel() is called before Prepare(), then it would just set the is_cance
This feels still unclear to me, especially the meaning of "it is an error".
What will be the consequence (crash, undef behavior, mem leak)? Is Prepare(),
Cancel(), d'tor() allowed?
Line 106: /// any rows).
> Calling Cancel() will set the RuntimeState to a cancelled state. It's hard
If Open sees it, it should just do nothing, right?
http://gerrit.cloudera.org:8080/#/c/4301/2/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
PS2, Line 44: This
Does "This" mean, that the profile is created during the teardown? How would I
retrieve it? If it just refers to the class, maybe reword "This class
maintains..."?
Line 74: return fragment_instance_id_;
single line?
PS2, Line 109: If this fragment instance has a sink
Does the caller still need to call GetNext() in this case to make the
destruction legal? Maybe we should add a comment that outlines the overall
lifetime of the instance state as a kind of state diagram (not sure if that
would help)? If Cancel() and destruction are the only async signals, we can
also make sure that their consequences are mentioned in each step of
Prepare()->Open()->GetNext().
PS2, Line 119: underlying plan fragment instance
What is this? I couldn't find it in the member variables. Should it say "Closes
this fragment instance state"? Do I have to call this after calling Cancel()?
Is it legal to call it before Open()/GetNext()?
PS2, Line 123: If called concurrently with Prepare(), will wait for
: /// Prepare() to finish in order to properly tear down
Prepare()'d state.
Another bit of the state machine that we seem to implement. Still not sure if
we should collect them centrally. Sailesh, Marcel, what's your opinion?
Line 160: /// set in ReportStatusCb();
Is this only ever set there?
PS2, Line 213: sent to this
"sent by this"? Or is this really the incoming sink? Maybe rename to
input_sink_ then?
PS2, Line 285: Does not set status_.
There's no status_ field. Maybe remove the comment altogether?
Line 300:
nit: remove blank line
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
PS1, Line 81: }
:
: QueryState* query_state() { return query_state_; }
:private:
: QueryState* query_state_;
: DISALLOW_COPY_AND_ASSIGN(Guard);
> We could but that would offload all the logic of getting the correct QS and
Ah, ok. Thanks for the explanation.
Line 112:
> num_active_finstances_ value is modified when not under a lock. It only kee
Maybe add a comment like "protected by xyz_lock_" and move it under that lock
together with the other protected fields?
http://gerrit.cloudera.org:8080/#/c/4301/2/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
Line 65: /// 1. If the QueryState exists, it will not be destroyed in the
scope of
nit: line wrapping
Line 84:private:
nit: newline above private: (not sure)?
PS2, Line 100: CancelPlanFragment
CancelFinstance?
PS2, Line 103: dst_instance_id
dst_finstance_id?
Line 142: /// Returns a pointer to the FInstanceState if one can be found for
the
Maybe say "non-owning pointer"?
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
PS1, Line 36:
> I used 'associated'.
Good
http://gerrit.cloudera.org:8080/#/c/4301/2/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
PS2, Line 21: boost
std/unordered_map?
Line 73: QueryState* GetQueryState(const TUniqueId& query_id);
Do these need to be public at all? Can we make them private and just use
QueryState::Guard? If we really need to move these pointers between scopes,
then adding and using QueryState::Guard::move() might be more explicit and
hence less error prone.
PS2, Line 84: fragments
fragment instances? Or really to all instances of a fragment? Maybe add (sic)
or some other wording to make this more clear.
PS2, Line 85: dst_instance_id
then this might need to become dst_finstance_id.
PS2, Line 96: boost
Can we use std::unordered_map?
Line 105: QueryState* FindOrCreateQueryState(const TQueryCtx& query_ctx);
Will the caller have to call ReleaseQueryState on it?
Line 107: /// This function is invoked once query_state is no longer in use,
and this
nit: "...once a query_stat
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide
execution state.
..
Patch Set 1:
(37 comments)
I've not addressed the comments in RuntimeState yet because I feel it's
tangential to this patch. I will get to addressing them once we've decided on
the headers.
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
Line 38: /// FInstanceState handles all aspects of the execution of a single
plan
> nit: line wrapping looks a bit off in this comment (and maybe elsewhere).
Done
PS1, Line 39: fragment
> in comments, please spell it out (as fragment instance)
Done
PS1, Line 39: fragment
> finstance
Done
PS1, Line 44: fragment
> finstance?
Done
Line 62: exec_params_(params) {
> do we need to copy these? that's a fairly large struct. you already get the
Yes, it looks like we can do without it. The only thing we need it for after
Prepare() is for the frarg_instance_id which I can store separately. I removed
it.
PS1, Line 68: /// It is an error to delete a FInstanceState before
Open()/GetNext() (depending
: /// on whether the fragment has a sink) indicated that
execution is finished.
> This part is unclear to me. Maybe it'll get clearer while reading through t
If Cancel() is called before Prepare(), then it would just set the
is_cancelled_ flag and return.
Prepare() on seeing this would return Status::CANCELLED.
Line 106: /// Start execution. Call this prior to GetNext().
> How does calling Cancel() affect Open()?
Calling Cancel() will set the RuntimeState to a cancelled state. It's hard to
guarantee if Open() will see it or not. It depends on what point of execution
Open() is in when the Cancel() happens.
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
Line 38: /// This object is created once the first fragment instance for a
query arrives to the
> arrives at
Done
Line 41: /// The lifetime of this class is maintained by using an explicit
reference count that is
> 'is dictated by an explicit ...' or 'is guided by an explicit ...'
Done
Line 67: /// fragment instances arrive in a single RPC.
> do we need this at all right now?
No we don't yet need this. Removed it.
Line 73: /// Delete all the FInstanceState objects. This is called when the
QueryState
> 'delete all state'. (no need to list that state, in particular since what t
Done
PS1, Line 79: RegisterFInstance
> Should this be renamed to include the fact that it does more than Register
The execution currently is done by the QueryExecMgr. I forgot to reword the
comment above. So this class only creates and registers a fragment instance.
PS1, Line 79: RegisterFInstance
> unless registration is a concept that shows up elsewhere in this class, no
This function would actually only create a fragment instance and register it
with the finstance_exec_state_map_.
The function would return and the QueryExecMgr would do the execution, since
that's what we decided.
The comment above was stale, sorry about that.
Line 81: /// Cancels a plan fragment that is running asynchronously.
> remove references to plan fragments, unless that's what you mean.
Done
PS1, Line 81: /// Cancels a plan fragment that is running asynchronously.
: Status CancelPlanFragment(const TUniqueId& finstance_id);
:
: /// Publishes a global runtime filter to a local fragment.
: Status PublishFilter(const TUniqueId& dst_instance_id, int32_t
filter_id,
: const TBloomFilter& thrift_bloom_filter);
> these look like forwarding methods. Would it be possible to make GetFInstan
We could but that would offload all the logic of getting the correct QS and FIS
from the query_id and frag_id, to the caller (who is ImpalaInternalService).
PS1, Line 111: QueryExecMgr
> mention it's a friend, so it's clear how it works without getters/setters?
Done
PS1, Line 111: controlled
> nit: used
Done
Line 112: int32_t num_current_references_;
> why is num_active_finstances_ atomic, but not this?
num_active_finstances_ value is modified when not under a lock. It only keeps
track of total fragments in flight. It can be used to display in the
RuntimeProfile and WebUI, it has no purpose other than that now.
num_current_references_ will always be modified under a lock. It keeps track of
total references to the QS (i.e. finstances and everything else)
Line 118: /// by us and its lifetime lasts as long as the QueryState that
owns it.
> we are that qs.
Done
Line 129: FInstanceState* GetFInstanceState(
> single line
Done
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/runtime-state-new.h
File be/src/runtime/runtime-state-new.h:
Line 1: // Licensed to the Apache Software Foundation
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Sailesh Mukil has uploaded a new patch set (#2). Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. This patch is a header preview of a query wide execution state for a backend. None of the existing headers are replaced, they are just added in parallel to existing headers and not considered for compilation. Changes in design are as follows: FragmentExecState -> FInstanceState This is now what was previously the FragmentExecState and the PlanFragmentExecutor together. The ReportStatusCallback is removed and now SendReport() is in charge of sending the report directly. The callback was in place originally only so the PlanFragmentExecutor could call into the FragmentExecState without a reference. This is not necessary anymore. PlanFragmentExecutor (collapsed into FInstanceState) FragmentMgr -> QueryExecMgr The QueryExecMgr now receives incoming fragments, creates a new QueryState if it is the first fragment to arrive for that query, and passes this received fragment along to the QueryState. This class is responsible for cleaning up the QueryState. QueryState (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 FInstanceStates 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 QueryState. Every user of the QueryState must access it within the scope of a 'Guard' object which guarantees ref count incrementing and decrementing at entry/exit of the scope. Excludes: - All .cc files to make use of changes made. Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed --- A be/src/runtime/fragment-instance-state.h A be/src/runtime/query-state.h M be/src/runtime/runtime-state.h A be/src/service/query-exec-mgr.h 4 files changed, 587 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/4301/2 -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide
execution state.
..
Patch Set 1:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
PS1, Line 39: fragment
> finstance
in comments, please spell it out (as fragment instance)
Line 62: exec_params_(params) {
do we need to copy these? that's a fairly large struct. you already get them in
Prepare(), maybe that's enough (or alternatively, pass them again into Open())
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
Line 38: /// This object is created once the first fragment instance for a
query arrives to the
arrives at
don't comment on the qem here. transfer those comments to the qem, if that's
needed.
Line 41: /// The lifetime of this class is maintained by using an explicit
reference count that is
'is dictated by an explicit ...' or 'is guided by an explicit ...'
Line 67: /// fragment instances arrive in a single RPC.
do we need this at all right now?
Line 73: /// Delete all the FInstanceState objects. This is called when the
QueryState
'delete all state'. (no need to list that state, in particular since what that
is will be changing soon.)
don't comment on the caller.
*do* comment on externally observable behavior (i'm assuming it's not legal to
call any other function of this class after this call).
PS1, Line 79: RegisterFInstance
> Should this be renamed to include the fact that it does more than Register
unless registration is a concept that shows up elsewhere in this class, no need
to mention it here (esp. not in the name).
i'd also avoid 'schedule', we already use that verb in a different context (and
i'm not sure what else we're trying to express other than 'execute' or 'start
fragment instance').
Line 81: /// Cancels a plan fragment that is running asynchronously.
remove references to plan fragments, unless that's what you mean.
Line 112: int32_t num_current_references_;
why is num_active_finstances_ atomic, but not this?
Line 118: /// by us and its lifetime lasts as long as the QueryState that
owns it.
we are that qs.
Line 129: FInstanceState* GetFInstanceState(
single line
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/runtime-state-new.h
File be/src/runtime/runtime-state-new.h:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
could you change runtime-state.h instead (and move the content into a new file
before check-in)? it's hard to see the diff.
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
Line 40: /// offer itself up for destruction when its reference count (i.e.
number of references
drop explanation of 'reference count'.
Line 56: /// Receives a remote fragment instance and creates or uses (if it
already exists) a
remove "receives a remote fragment instance" (what does that mean?)
Line 58: /// instance to it to be executed on a separate thread.
should this really be called 'StartFInstance'?
Line 61: /// was registered (like low memory). Otherwise, returns OK, which
guarantees that the
you talk about the fragment being 'registered', but it's unclear what that
means (because there are no related functions to that in this class - this
isn't a concept visible to the caller of this class)
Line 74: /// holds on to it. This is ensured by increasing the internal
reference count of the
don't explain implementation here.
but: it is a requirement that every caller of GetQueryState() eventually calls
ReleaseQueryState()
Line 84: Status CancelFInstance(const TUniqueId& finstanceance_id);
param spelling
Line 102: class Guard {
move this into QueryState. also, this needs to be public.
Line 140: /// Runs in the fragment instance' execution thread. Calls
ReleaseQueryState() to
don't comment on where this runs (the caller is in charge of that)
PS1, Line 138: /// Calls finstance_state->Prepare() and then
finstance_state->state->Exec().
: /// The finstance_state must previously have been registered.
: /// Runs in the fragment instance' execution thread. Calls
ReleaseQueryState() to
: /// decrease the refcount to the QueryState, which was
reserved during fragment instance
: /// registration time.
: void ExecInstance(QueryState* query_state, FInstanceState*
finstance_state);
> Should we move the whole method to the QueryState or even FInstanceState? I
this is presumably just a wrapper around the execution functionality in qs or
fis which handles the 'release' of the qs at the end.
name it accordingly (as an example, if this is an auxiliary function for the
function that starts the actual th
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide
execution state.
..
Patch Set 1:
(35 comments)
Please see my inline comments. Regarding the runtime-state I can see how it
would be more cache efficient the way it is right now. However I think it is
worth cleaning it up, and then addressing any subsequent cache-related
performance issues by re-grouping and re-ordering members of QueryState.
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
Line 38: /// FInstanceState handles all aspects of the execution of a single
plan
nit: line wrapping looks a bit off in this comment (and maybe elsewhere).
PS1, Line 39: fragment
finstance
PS1, Line 44: fragment
finstance?
PS1, Line 68: /// It is an error to delete a FInstanceState before
Open()/GetNext() (depending
: /// on whether the fragment has a sink) indicated that
execution is finished.
This part is unclear to me. Maybe it'll get clearer while reading through the
code. What if Cancel() is called before Prepare() even?
Line 106: /// Start execution. Call this prior to GetNext().
How does calling Cancel() affect Open()?
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
PS1, Line 79: RegisterFInstance
Should this be renamed to include the fact that it does more than Register the
instance (i.e. call methods on it)? ExecuteFInstance()?
ScheduleFinstanceExecution()? RegisterAndExecuteFInstance()?
PS1, Line 81: /// Cancels a plan fragment that is running asynchronously.
: Status CancelPlanFragment(const TUniqueId& finstance_id);
:
: /// Publishes a global runtime filter to a local fragment.
: Status PublishFilter(const TUniqueId& dst_instance_id, int32_t
filter_id,
: const TBloomFilter& thrift_bloom_filter);
these look like forwarding methods. Would it be possible to make
GetFInstanceState() public and let the caller call them?
PS1, Line 111: controlled
nit: used
PS1, Line 111: QueryExecMgr
mention it's a friend, so it's clear how it works without getters/setters?
http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/runtime-state-new.h
File be/src/runtime/runtime-state-new.h:
PS1, Line 74: cgroup
should we explain what these parameters do? It's not clear to me at all what
cgroup does.
PS1, Line 85: fragment
finstance?
PS1, Line 157: one
nit: maybe add 1 to...
Line 164: /// Returns runtime state profile
I don't think these getter comments are needed here - or we should add them
everywhere in the class.
Line 174: /// Returns codegen_ in 'codegen'. If 'initialize' is true,
codegen_ will be created if
Reword both sentences to "if 'initialize' is true and codegen_ has not been
initialized, it will be created and returned via 'codegen'." and "If
'initialize' is false and codegen_ has not been initialized, then 'codegen'
will be set to NULL."
PS1, Line 200: /// Log an error that will be sent back to the coordinator based
on an instance of the
: /// ErrorMsg class. The runtime state aggregates log messages
based on type with one
: /// exception: messages with the GENERAL type are not
aggregated but are kept
: /// individually.
: bool LogError(const ErrorMsg& msg, int vlog_level = 1);
:
: /// Returns true if the error log has not reached max_errors_.
: bool LogHasSpace() {
: boost::lock_guard l(error_log_lock_);
: return error_log_.size() < query_options().max_errors;
: }
:
: /// Returns the error log lines as a string joined with '\n'.
: std::string ErrorLog();
:
: /// Copy error_log_ to *errors
: void GetErrors(ErrorLogMap* errors);
:
: /// Append all accumulated errors since the last call to this
function to new_errors to
: /// be sent back to the coordinator
: void GetUnreportedErrors(ErrorLogMap* new_errors);
:
: /// Given an error message, determine whether execution should
be aborted and, if so,
: /// return the corresponding error status. Otherwise, log the
error and return
: /// Status::OK(). Execution is aborted if the ABORT_ON_ERROR
query option is set to
: /// true or the error is not recoverable and should be handled
upstream.
: Status LogOrReturnError(const ErrorMsg& message);
Can these go into an own struct/class?
PS1, Line 231: RuntimeProfile::Counter* total_cpu_timer() { return
total_cpu_timer_; }
: RuntimeProfile::Counter* total
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Lars Volker has posted comments on this change. Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. Patch Set 1: Can you try to find a time for a hangout so I can join the chat? Don't worry too much if it doesn't work out. In that case maybe you could send around a quick summary of what you discussed. Thanks -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. Patch Set 1: > > > (1 comment) > > > > @Matt: > > > > The RuntimeState has members that are relatively more frequently > > accessed than QueryState or FInstanceState members. > > So to have good cache temporal locality, we could leave it as a > > separate class. > > > > But to your point, it is a mess. Now that more people are > bringing > > this up, I feel having a RuntimeState per query wouldn't be a bad > > idea. We can move the fragment instance specific stuff to the > > FInstanceState and leave the query specific stuff in > RuntimeState. > > > > The QueryState on the other hand would just contain state > required > > for execution which relatively isn't accessed that frequently. > > > > Thoughts? > > Oh right, now I remember that cache locality had been brought up, > but I think we can probably work around it. (I guess one of the > issues is that there are larger structures, e.g. thrift > query/fragment descriptors? Those we can store on the heap, e.g. > via a scoped_ptr/unique_ptr on the QueryState/FInstanceState.) IMO > the RuntimeState is pretty gross and messes up your nice new > abstractions, so I think it'd be worth us chatting more about. > Perhaps we can chat in person next wk? Sure! Let's chat in person next week. -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. Patch Set 1: > > (1 comment) > > @Matt: > > The RuntimeState has members that are relatively more frequently > accessed than QueryState or FInstanceState members. > So to have good cache temporal locality, we could leave it as a > separate class. > > But to your point, it is a mess. Now that more people are bringing > this up, I feel having a RuntimeState per query wouldn't be a bad > idea. We can move the fragment instance specific stuff to the > FInstanceState and leave the query specific stuff in RuntimeState. > > The QueryState on the other hand would just contain state required > for execution which relatively isn't accessed that frequently. > > Thoughts? Oh right, now I remember that cache locality had been brought up, but I think we can probably work around it. (I guess one of the issues is that there are larger structures, e.g. thrift query/fragment descriptors? Those we can store on the heap, e.g. via a scoped_ptr/unique_ptr on the QueryState/FInstanceState.) IMO the RuntimeState is pretty gross and messes up your nice new abstractions, so I think it'd be worth us chatting more about. Perhaps we can chat in person next wk? -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. Patch Set 1: > (1 comment) @Matt: The RuntimeState has members that are relatively more frequently accessed than QueryState or FInstanceState members. So to have good cache temporal locality, we could leave it as a separate class. But to your point, it is a mess. Now that more people are bringing this up, I feel having a RuntimeState per query wouldn't be a bad idea. We can move the fragment instance specific stuff to the FInstanceState and leave the query specific stuff in RuntimeState. The QueryState on the other hand would just contain state required for execution which relatively isn't accessed that frequently. Thoughts? -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4301/1//COMMIT_MSG Commit Message: PS1, Line 47: runtime-state-new.h is what runtime-state.h would look like, and is : just a temporary name. Wouldn't we want to kill RuntimeState rather than update it to use the new QueryState/FInstanceState? I'd think that everything in RuntimeState could fit into either QueryState or FInstanceState. That would be really nice, because then the scope of those members is clear (whereas today RuntimeState is a mess of finstance & query state stuff, and even some wrappers around ExecEnv which may not be needed or maybe could just be accessed via QueryState). Sorry if I missed a discussion about this. -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. Patch Set 1: This addresses all the header comments in the non-header only patch: (i.e. https://gerrit.cloudera.org/#/c/3817/) I will post a compilable version of this patch in that CR. That will be a lot more tricky mainly because: - PlanFragmentExecutor and FragmentExecState are now one class. This makes the coordinator code tricky because the Coordinator had special code which depended only on the PFE and not on the FES; and has redundant FES functionality to manage the PFE. @Lars: This patch can be used as a guide for the per-query RPC (IMPALA-2550), and also for working out the final kinks in the headers. -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Sailesh Mukil has uploaded a new change for review. http://gerrit.cloudera.org:8080/4301 Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. .. IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. This patch is a header preview of a query wide execution state for a backend. None of the existing headers are replaced, they are just added in parallel to existing headers and not considered for compilation. Compiling the code will work and is no different from the current apache/master branch (i.e. the commit before this patch). Changes in design are as follows: FragmentExecState -> FInstanceState This is now what was previously the FragmentExecState and the PlanFragmentExecutor together. The ReportStatusCallback is removed and now SendReport() is in charge of sending the report directly. The callback was in place originally only so the PlanFragmentExecutor could call into the FragmentExecState without a reference. This is not necessary anymore. PlanFragmentExecutor (collapsed into FInstanceState) FragmentMgr -> QueryExecMgr The QueryExecMgr now receives incoming fragments, creates a new QueryState if it is the first fragment to arrive for that query, and passes this received fragment along to the QueryState. This class is responsible for cleaning up the QueryState. QueryState (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 FInstanceStates 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 QueryState. Every user of the QueryState must access it within the scope of a 'Guard' object which guarantees ref count incrementing and decrementing at entry/exit of the scope. runtime-state-new.h is what runtime-state.h would look like, and is just a temporary name. Excludes: - All .cc files to make use of changes made. Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed --- A be/src/runtime/fragment-instance-state.h A be/src/runtime/query-state.h A be/src/runtime/runtime-state-new.h A be/src/service/query-exec-mgr.h 4 files changed, 981 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/4301/1 -- To view, visit http://gerrit.cloudera.org:8080/4301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil
