Marcel Kornacker has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. ......................................................................
Patch Set 3: (20 comments) http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/fragment-exec-state.h File be/src/runtime/fragment-exec-state.h: Line 72: // TODO: make pointer to shared per-query state better yet: get rid of this and provide a convenience function that goes through query_exec_state_ you can do this now, this shouldn't be many more lines of code than what you're removing. (if a todo is trivial to resolve, it's good practice to do that right away.) Line 77: // TODO: make pointer to shared per-query state same here http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: Line 209: request.fragment_ctx.fragment.plan, runtime_state_->query_exec_state()->desc_tbl(), why go through runtime_state_? http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: Line 44: /// Initializes common data structures (descriptor table, ...). This method can be precede w/ blank line Line 56: Status AcceptIncomingFragment(const TExecPlanFragmentParams& exec_params); what does 'accept' mean? are there any non-incoming fragments? are these really fragment instances? Line 63: void PublishFilter(TPublishFilterResult& return_val, follow conventions for output param Line 84: const TQueryCtx& query_ctx_; who owns this? Line 91: int32_t num_active_references_; are there inactive references? Line 97: /// referenced as a shared_ptr to allow asynchronous calls to CancelPlanFragment() why do async calls require a shared_ptr? Line 99: FragmentExecStateMap; indentation Line 111: void FragmentThread(TUniqueId fragment_instance_id); const ref http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.cc File be/src/service/fragment-mgr.cc: Line 42: TUniqueId query_id = exec_params.query_ctx.query_id; const ref http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.h File be/src/service/fragment-mgr.h: Line 42: : destroy_thread_("QueryExecMgr", "DestroyThread", 1, 1, please move function bodies into the .cc file unless needed in the .h for performance. this reduces the rebuild footprint of a code change. Line 45: /// Receives a remote fragment and adds it to the fragment_instance_query_id_map_. don't reference private state in the public interface. the function comment should completely describe the observable behavior, but not the implementation. distinguish between fragments and their instances. all execution requires an instance. Line 65: /// and if the QueryExecState is not already scheduled for destruction. what are the lifetime guarantees of the returned state? does the caller need to release it? Line 70: void ReturnQueryExecState(QueryExecState* query_exec_state); 'release' is a more common term for this operation Line 73: /// fragment intance id. Otherwise the pointer will contain NULL. spelling same question about what is required of the caller PS3, Line 81: class ScopedQueryExecStateRef { > I don't know if this is a good way, or if we should have every caller call we probably need both. having this is the preferable way where the reference stays within a single block. Line 84: QueryExecMgr& query_exec_mgr, QueryExecState* query_exec_state) : don't use ref params given that your GetQueryExecState functions don't return Status, why not embed the call in the c'tor; ie: Scoped...(QueryExecMgr* mgr, const TUniqueId& query_id); Line 102: /// reference counter to track the number or currently running fragment instances. When 'number of'? also, why does the ref counter only track the currently running instances? i'd assume that every accessor will need to increase the ref counter. -- 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: 3 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: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
