Lars Volker has posted comments on this change. Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context. ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/3686/2/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: Line 22: class QueryExecState { > +1 to all of these suggestions (but wait to do any giant rename until this Yes, I'll hold off until we got the rest sorted out and will rename things later. Line 24: QueryExecState(const TExecPlanFragmentParams& params); > since this is shared between all fragment instances of a query, i wouldn't I changed the c'tor to take const TQueryCtx&, it will probably need to take more parameters in the future. Regarding public/private c'tors, is there a rule of thumb? Do we do this for all classes that have a single consumer (make the c'tor private, make the consumer friend)? Line 41: DescriptorTbl* desc_tbl_; > who owns this? Following the pattern seen elsewhere it'd be obj_pool_. I added a comment. http://gerrit.cloudera.org:8080/#/c/3686/2/be/src/service/fragment-exec-state.h File be/src/service/fragment-exec-state.h: Line 71: TQueryCtx query_ctx_; > these can all become references (into the original exec request stored in t I added a comment. Line 72: TPlanFragmentInstanceCtx fragment_instance_ctx_; This is per instance, so it would still only be needed here. http://gerrit.cloudera.org:8080/#/c/3686/2/be/src/service/fragment-mgr.h File be/src/service/fragment-mgr.h: PS2, Line 50: Status ExecPlanFragment(const TExecPlanFragmentParams& params); > when you have batching you should have a method like ExecFragmentsForQuery( I'm reluctant to introduce a method for batched instance startup in this change, since I assume it would make it necessary to update the thrift structures, similar to the previous change I worked on: https://gerrit.cloudera.org/#/c/3390/5/common/thrift/ImpalaInternalService.thrift@379 Instead we could keep ExecPlanFragment() and create the QueryExecState from there, possibly in a helper method. I'll add such a method in the next PS. Line 79: /// the last fragment exits we remove the corresponding QueryExecState. > then why have a shared_ptr (instead of an explicit function to register a r I used a shared_ptr solely for consistency with other maps. The alternative would be making QueryExecState move'able (but not copy'able), or use unique_ptr<QueryExecState>, so we can store it in std::unordered_map. Yet another option would be using an object_pool and storing pointers in the map. Which one should we prefer, technically, to store the elements in the map? This should be orthogonal to the lifetime considerations below. PS2, Line 97: boost: > std::unordered_map Done -- To view, visit http://gerrit.cloudera.org:8080/3686 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c513d08f718699ba0c4cdb90c117aaecf95d7fc Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[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-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
