Marcel Kornacker has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. ......................................................................
Patch Set 5: (25 comments) 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 separate abstractions (and if so, what are the differentiating characteristics)? Line 40: boost::mem_fn(&FragmentExecState::ReportStatusCb), lambda Line 42: client_cache_(exec_env->impalad_client_cache()), exec_params_(params) { break into two lines, all other members are on separate lines Line 80: // TODO: make pointer to shared per-query state what does this todo mean? 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? http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: Line 56: const DescriptorTbl& desc_tbl() const { return *desc_tbl_; } > 'Accept' was just a way to say that it was getting it from the QueryExecMgr my point was: don't use superfluous ('incoming': all fragment instances are incoming) words or words that convey no meaning ('accept'?) in names, they don't help the reader of your code. 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()? Line 79: QueryExecMgr* query_exec_mgr_; presumably "not owned" Line 85: ObjectPool* obj_pool_; owner? 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 shared_ptrs. anyone with an active FragmentExecState should also be included in the ref count. also, this doesn't need to be a map, you can use an array/vector instead (and use instance_id - query_id as the index. Line 108: std::shared_ptr<FragmentExecState> GetFragmentExecState( remove shared_ptr or unique_ptr since we're now switching to an explicit mem mgmt model Line 115: void FragmentInstanceExecThread(const TUniqueId& fragment_instance_id); why not just ExecInstance? 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". 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 reference counting mechanism. Line 59: /// no-op (because the fragment is unregistered). you're also introducing terminology 'registered' and 'unregistered'. what are the semantics of that? is this distinct from live/refcount > 0 and not-live/refcount == 0? Line 60: Status RegisterFragmentInstanceWithQuery(const TExecPlanFragmentParams& params); why doesn't this return the qes? you'd typically want to access that right after this call. 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 describe the externally observable behavior of a function, and a change in the refcount is one of the main side effects of calling this function). this is also missing elsewhere. 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 class comment. also, i couldn't find the reference count anywhere so far. also, why isn't this a member function of qes itself? Line 75: Status GetQueryId(const TUniqueId& fragment_instance_id, TUniqueId* query_id); why do we need this? 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 guarantees unlocking/decreasing the refcount, but it doesn't mediate access. what you have will get unwieldy very quickly. 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. transfer ownership where? Line 99: class ScopedQueryExecStateRef { why is this local to qem? Line 124: QueryExecMgr* query_exec_mgr_; why do you need this? Line 126: const TUniqueId query_id_; do you need this if query_exec_state_ is null? Line 137: typedef boost::unordered_map<TUniqueId, std::unique_ptr<QueryExecState>> why unique_ptr instead of just a naked pointer? -- 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: 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
