Sailesh Mukil has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. ......................................................................
Patch Set 3: (23 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 th Done Line 77: // TODO: make pointer to shared per-query state > same here This got carried over from Lars patch. I had overlooked it the last time. Do you mean I should create a shared client_cache between all the fragments vs. a single client cache per fragment? 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_? I've changed it to just access it from its own pointer 'query_exec_state_'. http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/runtime/query-exec-state.cc File be/src/runtime/query-exec-state.cc: PS3, Line 72: QueryExecMgr::ScopedQueryExecStateRef s(*query_exec_mgr, this); > Bug: I would have to call GetQES(query_ctx_.query_id) before this line to i This bug no longer exists as I call GetQES() inside the ScopedQueryExecStateRef() c'tor. However, I still feel this is somewhat suboptimal. 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 Done Line 56: Status AcceptIncomingFragment(const TExecPlanFragmentParams& exec_params); > what does 'accept' mean? are there any non-incoming fragments? are these re 'Accept' was just a way to say that it was getting it from the QueryExecMgr. I said 'Incoming' because it came in as a remote fragment instance. In any case I've changed the function name to RegisterFragmentInstance(). Line 63: void PublishFilter(TPublishFilterResult& return_val, > follow conventions for output param Done Line 84: const TQueryCtx& query_ctx_; > who owns this? It's owned by this class. Removed the const ref. Line 91: int32_t num_active_references_; > are there inactive references? No, changed it to num_current_references_. Line 97: /// referenced as a shared_ptr to allow asynchronous calls to CancelPlanFragment() > why do async calls require a shared_ptr? I didn't write a lot of this code (I moved it from fragment-exec-state), but looking at the code the shared_ptr is required because if CancelPlanFragment() is called for a fragment and just while beginning the cancellation, if the fragment completes executing, it will be removed from the FragmentExecStateMap causing the FragmentExecState to go out of scope. So accessing it as a shared_ptr ensures that it's still available for the scope of CancelPlanFragment() Line 99: FragmentExecStateMap; > indentation Done Line 111: void FragmentThread(TUniqueId fragment_instance_id); > const ref Done 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 Done http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/fragment-mgr.h File be/src/service/fragment-mgr.h: Line 28: /// Manages execution of individual plan fragment instances, which are typically run as a > as part of the header preview, please also rewrite the class comments to fu Sorry about that. I've made the change here and elsewhere. Line 42: : destroy_thread_("QueryExecMgr", "DestroyThread", 1, 1, > please move function bodies into the .cc file unless needed in the .h for p Done 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 Done Line 65: /// and if the QueryExecState is not already scheduled for destruction. > what are the lifetime guarantees of the returned state? does the caller nee Done Line 70: void ReturnQueryExecState(QueryExecState* query_exec_state); > 'release' is a more common term for this operation Done Line 73: /// fragment intance id. Otherwise the pointer will contain NULL. > spelling Done PS3, Line 81: class ScopedQueryExecStateRef { > we probably need both. Done Line 84: QueryExecMgr& query_exec_mgr, QueryExecState* query_exec_state) : > don't use ref params Done. Since every reference is per fragment, I've made the second parameter 'fragment_instance_id' instead of 'query_id', so that we don't need to look up another map to find the query_id before calling this. 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 runnin It tracks every accessor, I've changed the comment to reflect that. http://gerrit.cloudera.org:8080/#/c/3817/3/be/src/service/request-exec-state.h File be/src/service/request-exec-state.h: Line 55: class ImpalaServer::RequestExecState { > request is very generic. this is specifically for client requests, so Clien Done. Will do a more thorough walkthrough and rename members after the header patch as a whole is decided on. -- 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
