impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) MT: Planner for multi-threaded execution
Date Wed, 04 May 2016 20:39:12 GMT
Henry Robinson has posted comments on this change.

Change subject: MT: Planner for multi-threaded execution
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/2846/5/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 188: mt_num_cores
It seems like a bad idea to ship this option. Users will see it (it will turn up in the shell).


http://gerrit.cloudera.org:8080/#/c/2846/5/fe/src/main/java/com/cloudera/impala/common/TreeNode.java
File fe/src/main/java/com/cloudera/impala/common/TreeNode.java:

Line 30: protected TreeNode() {}
is this still necessary?


http://gerrit.cloudera.org:8080/#/c/2846/5/fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java
File fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java:

Line 32: sequence
also 'tree' here, no?


Line 35: P
remove (you don't refer to P anywhere else)


Line 35: In other words, the assignment
       :  * to cohorts represents the tree structure of the pla
IIUC, there's one bit of information that would make this more clear: one plan may receive
many *different* build sides if it contains more than one join node. Understanding this point
makes the whole thing clearer, because now I see why cohorts may a) have more than one member
and b) represent some kind of parallel unit of execution.

I'd rephrase the previous sentence like this:

"All plans that produce intermediate join build sides (one per join node in the recipient)
for a single recipient plan are grouped together into a 'cohort'. Since each plan may only
produce a build side for at most one recipient plan, each plan belongs to exactly one cohort."

and remove the following sentence about representing the tree structure of the plan, which
I don't feel adds anything. 

Sorry to harp on the comments, but from the perspective of someone who doesn't have deep expertise
in the FE, the comments are the first place to understand this class.


-- 
To view, visit http://gerrit.cloudera.org:8080/2846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3c34dd3f9190a131e6f03d901b4bfcd164a5174
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message