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 (ASF) under one > could you change runtime-state.h instead (and move the content into a new f Done http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/service/query-exec-mgr.h File be/src/service/query-exec-mgr.h: Line 31: /// Manages execution of queries by creating a QueryState object which owns > Mention that exactly one instance of this exists per process? I think that Done PS1, Line 36: passed > Can we find a better word here? 'passed' seems to imply that it's created o I used 'associated'. Line 40: /// offer itself up for destruction when its reference count (i.e. number of references > drop explanation of 'reference count'. Done 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?) I meant an incoming fragment instance from the coordinator. Reworded it now. Line 58: /// instance to it to be executed on a separate thread. > should this really be called 'StartFInstance'? Yes, that makes more sense. Changed it. 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 You're right. I removed the use of the word 'register'. I reworded to use 'start' instead. Line 74: /// holds on to it. This is ensured by increasing the internal reference count of the > don't explain implementation here. Done Line 84: Status CancelFInstance(const TUniqueId& finstanceance_id); > param spelling Done PS1, Line 95: class object > what does "class object" mean? Just meant 'class'. Done. Line 102: class Guard { > move this into QueryState. also, this needs to be public. Done Line 105: query_state_ = ExecEnv::GetQueryExecMgr()->GetQueryState(query_id); > Adding DCHECK(ExecEnv::GetQueryExecMgr() != nullptr) might help spot issues Done Line 109: ExecEnv::GetQueryExecMgr()->ReleaseQueryState(query_state_); > Same DCHECK here, plus it might catch errors with the order in which object Done PS1, Line 119: exits > nit: is destructed? An accessor doesn't have to be destroyed to drop a reference. I reworded it to "drops its reference". 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) Done 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 I added a comment why it has to be here. 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); > this is presumably just a wrapper around the execution functionality in qs This function does not start the thread. This is the function that gets run by the thread when it is spawned. Currently the thread gets started in StartFInstance(). I changed the name to ExecFinstanceAux(). -- 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 <[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
