Marcel Kornacker has posted comments on this change. Change subject: MT: Planner for multi-threaded execution ......................................................................
Patch Set 4: (15 comments) http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/common/TreeNode.java File fe/src/main/java/com/cloudera/impala/common/TreeNode.java: Line 25: * Generic tree structure. Only concrete subclasses of this can be instantiated. > Why not mark the class as abstract then? Done http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java File fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java: Line 145: output.append( > indent two spaces Done http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java File fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java: Line 32: * (= sequence of plan fragments) into a sequence of distributed plans. One of those > 'sequence' is misleading. Whether or not the fragments *can* be sequenced b Done Line 34: * The plans that produce intermediate join build sides for a particular plan are > This could be clearer: what's the "particular plan" in this context? Does t Done Line 35: * grouped together into a cohort. > Comment on why the cohort abstraction is relevant (is it a single schedulab i want to do that when i actually implement the scheduling. right now it just expresses a producer/consumer relationship, and how exactly we use that for scheduling is tbd. Line 135: * fragments required for this materialization from tree rooted at parentFragment into > parentFragment isn't defined anywhere Done Line 141: // collect all ExchangeNodes on the build side and their corresponding input fragments > It's more readable if you capitalise sentences. most of our single-sentence comments aren't capitalized and are missing a closing '.' Line 145: com.google.common.base.Predicate<PlanFragment> isInputFragment = > import com.google.common.base.Predicate ? i like this better because i can see right away what predicate i'm looking at (because we're also dealing with our own predicates). it doesn't add more lines of code so i find it acceptable. Line 161: // Create new fragment with JoinBuildSink that consumes the materialization > JoinBuildsSink ? Line 162: // output. The new fragment has the same data partition as the join node's fragment. > 'materialization output' is unclear here: is it materialized before it gets Done Line 169: join.getFragment().getDataPartition()); > This seems like it could go on two lines? Done http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/planner/PlanFragment.java File fe/src/main/java/com/cloudera/impala/planner/PlanFragment.java: Line 56: * is produced by the plan root is marked as materialized. > I think these relationships would be easier to understand if they were made this could be done by having an explicit Plan class that was also a TreeNode. i don't want to embark on that just yet, though. i'll leave a todo. Line 60: * fragment (or, in the case of the root fragment, is materialized in some form). > not true if the output is repartitioned (or at least, not clear that this i it's still true, we're not doing multi-instance execution yet. Line 371: // all ExchangeNodes have registered input fragments > this seems more like a well-formedness property of a plan, rather than a pl plan = tree of PlanFragments, which is why this is called verifyTree. maybe it should be called something else? http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: Line 950: if (RuntimeEnv.INSTANCE.isTestEnv() > whitespace Done -- 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: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
