Sailesh Mukil has posted comments on this change.

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

Patch Set 4:


Thanks for the review Lars. I've made the changes.
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() { }
> This feels still unclear to me, especially the meaning of "it is an error".
It will be undefined behavior because if execution is not complete, the 
FInstanceState will be expected to be present in the map.

Line 106:   /// is > 0 and a callback was specified in the c'tor.
> If Open sees it, it should just do nothing, right?
Yes, Open() does nothing special if Cancel() happens.
File be/src/runtime/fragment-instance-state.h:

PS2, Line 44: This
> Does "This" mean, that the profile is created during the teardown? How woul
Yes, it means this class. Done.

Line 74:   const TNetworkAddress& coord_address() const { return 
query_ctx_.coord_address; }
> single line?

PS2, Line 109: us Open();
> Does the caller still need to call GetNext() in this case to make the destr
That sounds like a good idea. I unfortunately don't know all the possibilities 
well enough to draw the diagram. I will sit down with someone who does and add 
this state diagram.

PS2, Line 119: 
> What is this? I couldn't find it in the member variables. Should it say "Cl
That comment was copied over from the old PFE. Changed it.

This is called in the destructor, so it doesn't need to be explicitly called 
after Cancel().

It doesn't seem legal to call before Open()/GetNext() but I'm not sure. Will 
confirm and update.

PS2, Line 123: 
             :   /// Cancel() may be called more than once. Calls after the 
first will hav
> Another bit of the state machine that we seem to implement. Still not sure 
I don't understand what you mean by "collect them centrally". Could you clarify?

Line 160: 
> Is this only ever set there?
No, it was previously. But now it will be set in multiple places. So I've 
removed the comment.

PS2, Line 213: k> sink_;
> "sent by this"? Or is this really the incoming sink? Maybe rename to input_
I've rephrased this. "sent to/by this" leaves room for ambiguity.

PS2, Line 285:  sink, the sink will
> There's no status_ field. Maybe remove the comment altogether?
It should be exec_status_. Since we've merged both the classes, I've gotten rid 
of status_ and kept only exec_status_.

Line 300: 
> nit: remove blank line
File be/src/runtime/query-state.h:

Line 112:   }
> Maybe add a comment like "protected by xyz_lock_" and move it under that lo
File be/src/runtime/query-state.h:

Line 65:   ///   1. If the QueryState exists, it will not be destroyed in the 
scope of this class
> nit: line wrapping

Line 84: 
> nit: newline above private: (not sure)?

PS2, Line 100: cels a plan fragme
> CancelFinstance?

PS2, Line 103: to a local frag
> dst_finstance_id?

Line 142:   FInstanceStateMap finstance_exec_state_map_;
> Maybe say "non-owning pointer"?
This pointer is owned by QueryState however.
File be/src/service/query-exec-mgr.h:

PS2, Line 21: unord
> std/unordered_map?

Line 73:   /// Forwards a global runtime filter to the corresponding QueryState 
to publish to
> Do these need to be public at all? Can we make them private and just use Qu
This doesn't need to be public now. Made both this and ReleaseQS() private.

PS2, Line 84: eryState 
> fragment instances? Or really to all instances of a fragment? Maybe add  (s
Since we have the instance_id as the parameter, this should be fragment 
instance. Done.

PS2, Line 85: 
> then this might need to become dst_finstance_id.

PS2, Line 96: s on 
> Can we use std::unordered_map?

Line 105:   /// Caller must call ReleaseQueryExecState() once done with this.
> Will the caller have to call ReleaseQueryState on it?
Yes. Done.

Line 107: 
> nit: "...once a query_state..."

PS2, Line 112: 
> finstance_state doesn't seem to have a 'state' field. Should this be finsta
It should become Open(). Changed it.

PS2, Line 117: once we 
> typo

PS2, Line 116:   /// reserved during fragment instance registration time.
             :   /// TODO: Move this responsibility outside this class once we 
have per-query RPC. This
> Isn't this also called here because we want the QEM to start the threads? I
Yes, but we want the QEM to start the threads because we want the QEM to manage 
the refcounts.
We will later move this responsibility to the QS.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
Gerrit-PatchSet: 4
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 <>
Gerrit-HasComments: Yes

Reply via email to