impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Date Thu, 21 Jul 2016 23:11:55 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message