Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state. ......................................................................
Patch Set 1: (21 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 39: fragment > finstance in comments, please spell it out (as fragment instance) Line 62: exec_params_(params) { do we need to copy these? that's a fairly large struct. you already get them in Prepare(), maybe that's enough (or alternatively, pass them again into Open()) 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 don't comment on the qem here. transfer those comments to the qem, if that's needed. 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 ...' Line 67: /// fragment instances arrive in a single RPC. do we need this at all right now? 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 that is will be changing soon.) don't comment on the caller. *do* comment on externally observable behavior (i'm assuming it's not legal to call any other function of this class after this call). PS1, Line 79: RegisterFInstance > Should this be renamed to include the fact that it does more than Register unless registration is a concept that shows up elsewhere in this class, no need to mention it here (esp. not in the name). i'd also avoid 'schedule', we already use that verb in a different context (and i'm not sure what else we're trying to express other than 'execute' or 'start fragment instance'). Line 81: /// Cancels a plan fragment that is running asynchronously. remove references to plan fragments, unless that's what you mean. Line 112: int32_t num_current_references_; why is num_active_finstances_ atomic, but not this? Line 118: /// by us and its lifetime lasts as long as the QueryState that owns it. we are that qs. Line 129: FInstanceState* GetFInstanceState( single line 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 file before check-in)? it's hard to see the diff. http://gerrit.cloudera.org:8080/#/c/4301/1/be/src/service/query-exec-mgr.h File be/src/service/query-exec-mgr.h: Line 40: /// offer itself up for destruction when its reference count (i.e. number of references drop explanation of 'reference count'. 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?) Line 58: /// instance to it to be executed on a separate thread. should this really be called 'StartFInstance'? 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 means (because there are no related functions to that in this class - this isn't a concept visible to the caller of this class) Line 74: /// holds on to it. This is ensured by increasing the internal reference count of the don't explain implementation here. but: it is a requirement that every caller of GetQueryState() eventually calls ReleaseQueryState() Line 84: Status CancelFInstance(const TUniqueId& finstanceance_id); param spelling Line 102: class Guard { move this into QueryState. also, this needs to be public. 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) 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 this is presumably just a wrapper around the execution functionality in qs or fis which handles the 'release' of the qs at the end. name it accordingly (as an example, if this is an auxiliary function for the function that starts the actual thread, you could name it '..Aux'). -- 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
