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 <sail...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
