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_state..." PS2, Line 112: finstance_state->state->Exec() finstance_state doesn't seem to have a 'state' field. Should this be finstance_state->Open() or Prepare()? PS2, Line 116: /// TODO: Move this responsibility outside this class once we have per-query RPC. This : /// is currently called from here because we want the recounts to be managed in the QEM. Isn't this also called here because we want the QEM to start the threads? If so, remove the TODO and just keep this function here. PS2, Line 117: recounts typo -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
