impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.
Date Mon, 12 Sep 2016 20:27:24 GMT
Sailesh Mukil has posted comments on this change.

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


Patch Set 4:

(27 comments)

Thanks for the review Lars. I've made the changes.

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() { }
> 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.


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 woul
Yes, it means this class. Done.


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


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
Done


http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 112:   }
> Maybe add a comment like "protected by xyz_lock_" and move it under that lo
Done


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 this
class
> nit: line wrapping
Done


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


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


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


Line 142:   FInstanceStateMap finstance_exec_state_map_;
> Maybe say "non-owning pointer"?
This pointer is owned by QueryState however.


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: unord
> std/unordered_map?
Done


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.
Done


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


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..."
Done


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
Done


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 http://gerrit.cloudera.org:8080/4301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@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