Sailesh Mukil has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. ......................................................................
Patch Set 5: (33 comments) http://gerrit.cloudera.org:8080/#/c/3817/5//COMMIT_MSG Commit Message: PS5, Line 15: recieves > receives Done PS5, Line 17: > A comma would be helpful here. Done PS5, Line 24: and is > "and it is" ? Done PS5, Line 24: which > This seems like an incomplete thought. Perhaps the "which" just needs to be Done http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/fragment-exec-state.h File be/src/runtime/fragment-exec-state.h: Line 77: TPlanFragmentInstanceCtx fragment_instance_ctx_; > remove, unless there's something to do Done http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/fragment-exec-state.h File be/src/runtime/fragment-exec-state.h: Line 32: /// Execution state of a single plan fragment. > what differentiates this from RuntimeState. ie, are these really two separa The RuntimeState contains mostly the members populated at runtime like the QueryMemTracker, runtime profile counters, block managers, etc. The FragmentExecState mostly has execution state associated with it and sets up the PlanFragmentExecutor. If we were to merge them, that should be done as a separate patch as it would be quite a big change. However, I feel that would make the class pretty bloated. PS5, Line 33: FragmentExecState > Are we going to rename this to make it clear that it's instance-specific st Yes, I've renamed it. Line 40: boost::mem_fn(&FragmentExecState::ReportStatusCb), > lambda Done Line 42: client_cache_(exec_env->impalad_client_cache()), exec_params_(params) { > break into two lines, all other members are on separate lines Done Line 80: // TODO: make pointer to shared per-query state > what does this todo mean? I forgot to remove that. Removed it now. http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/plan-fragment-executor.h File be/src/runtime/plan-fragment-executor.h: Line 64: class PlanFragmentExecutor { > needs new name. FragmentInstanceExecutor? Done http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: Line 55: ObjectPool* obj_pool() const { return obj_pool_.get(); } > .get()? Oops, removed it. Line 79: QueryExecMgr* query_exec_mgr_; > presumably "not owned" Done Line 85: ObjectPool* obj_pool_; > owner? Done Line 101: /// referenced as a shared_ptr to allow asynchronous calls to CancelPlanFragment() > why do we need a shared_ptr? we should get rid of implicit mem mgmt with sh Having shared_ptrs are how it was done before. I've changed it to use raw pointers now though with an explicit mem mgmt model. Are fragment instance numbers guaranteed to be consecutive in a backend? If not, we will have gaps in the vector/array. Line 108: std::shared_ptr<FragmentExecState> GetFragmentExecState( > remove shared_ptr or unique_ptr since we're now switching to an explicit me Done Line 115: void FragmentInstanceExecThread(const TUniqueId& fragment_instance_id); > why not just ExecInstance? Done http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/client-request-exec-state.h File be/src/service/client-request-exec-state.h: Line 44: /// Execution state of a query. This captures everything necessary > clarify comment. this isn't just the "execution state". Done http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/query-exec-mgr.h File be/src/service/query-exec-mgr.h: PS4, Line 80: eId& fragment_instance_id); > we can also encode the instance id as query id + instance offset/instance n Yes I saw the patch for IMPALA-3988. This function has been removed in any case in the next patch set. Line 91: /// The caller should always check if obj->is_valid_ref() is 'true' before using it. > please find a more concise name (maybe take inspiration from 'lock_guard'?) Renamed it to QESGuard. Done. http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/query-exec-mgr.h File be/src/service/query-exec-mgr.h: Line 40: /// offer itself up for destruction when it is no longer being used and the > 'no longer being used' is vague. mention (and make concrete in code) the re Done Line 59: /// no-op (because the fragment is unregistered). > you're also introducing terminology 'registered' and 'unregistered'. what a The registered and unregistered semantics have already been in use. 'Registered' means it has been successfully created and put in the map. 'Unregistered' means that it cannot be found on the map any longer. Now with the introduction of ref counts, it is the same as refcount>0 and refcount==0, because if it exists on the map, the refcount>0 and once the refcount==0, it's immediately removed from the map. Line 60: Status RegisterFragmentInstanceWithQuery(const TExecPlanFragmentParams& params); > why doesn't this return the qes? you'd typically want to access that right This is called from the impala-internal-service, so there wouldn't be any use in returning the QES back to it. This function stores the newly created QES in the map and calls QES->RegisterFragmentInstance(). There is no access to the QES in the context of this thread after that. Line 62: /// Return a pointer to the QueryExecState if one can be found for the given query id > describe the effect on the reference count (a function comment should descr Done Line 69: /// Decrements query_exec_state->num_active_references and schedules it for destruction > reference counting is a central concept here, it needs to be described in a This class is the QueryExecMgr. The reference count belongs to the QueryExecState, i.e. the num_current_references counter. This function belongs to the QueryExecMgr because we need to hold the query_exec_state_map_lock_ while releasing the QES back to the map. Line 75: Status GetQueryId(const TUniqueId& fragment_instance_id, TUniqueId* query_id); > why do we need this? I removed this in the last patchset, forgot to clear it form the header file. Done. Line 87: /// This class c'tor takes a query_id and guarantees the caller one of two things: > i think something like a lock_guard might be more appropriate - it guarante I've removed the access mediation via functions. Now this just increases/decreases the ref count. Line 97: /// TODO: Is this really necessary? The downside is that this class needs to duplicate > i don't understand the purpose of preventing access to the qes. I was worried that maybe in the future, code could be added in a way where the QES is passed to another thread which would break the refcount semantics. But like you said it could get unwieldy very quick. Also this way is not completely foolproof (there are other ways to pass the QES to another thread if someone really wants to). Line 99: class ScopedQueryExecStateRef { > why is this local to qem? Because it essentially does a GetQES() and a ReleaseQES() which are QEM functions. Do you mean we should have this in a separate class? Line 124: QueryExecMgr* query_exec_mgr_; > why do you need this? GetQES() and ReleaseQES() are member functions of the QEM. We need to access those functions through query_exec_mgr_. Line 126: const TUniqueId query_id_; > do you need this if query_exec_state_ is null? This is actually not needed. It's only required in the constructor. I've removed it. Line 137: typedef boost::unordered_map<TUniqueId, std::unique_ptr<QueryExecState>> > why unique_ptr instead of just a naked pointer? To give it a definite lifetime, but since we're explicitly managing lifetime with refcounts, a naked pointer makes more sense. I've changed it. http://gerrit.cloudera.org:8080/#/c/3817/5/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 499: > spaces Done -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
