drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Venki Korukanti" <venki.koruka...@gmail.com>
Subject Re: Review Request 30466: DRILL-133: LocalExchange (planning and parallelization)
Date Wed, 04 Feb 2015 02:21:50 GMT


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java,
line 33
> > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line33>
> >
> >     It looks like endpoint should be final.

updated.


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java,
line 45
> > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line45>
> >
> >     If I created this for a particular endpoint, why would I want to be able to
change it?
> >     
> >     If I do change it, why doesn't that invalidate the affinity value?

This method is not needed. Removed it.


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java,
line 54
> > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line54>
> >
> >     What does it mean for the two endpoint values not to be equal? How do you expect
this to be used?

We use this for sorting the EndpointAffinity list based on the affinity. Added a comment.


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java,
line 66
> > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line66>
> >
> >     Can this condition ever occur? It it actually possible to reach POSITIVE_INFINITY
by adding to the affinity as in addAffinity() (and the condition in there also looks suspicious)?

We use POSITIVE_INIFINITY to indicate that endpoint in this object should have an assignment.
One example is Screen. Fragment containing Screen should always be assigned to the node where
the Foreman for the query is present.


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractExchange.java,
line 29
> > <https://reviews.apache.org/r/30466/diff/1/?file=842670#file842670line29>
> >
> >     Why are these not final (and set in an AbstractExchange constructor)?
> >     
> >     Is the MuxExchange below, the sender major fragment id is referenced, but has
never been set. It looks like these aren't both used. Perhaps they shouldn't be declared in
this class, but each class that does need them should declare whichever one it needs itself.

These are set when assigning the sender endpoint list in AbstractExchange.setupSenders() (similar
method for receiver)


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScan.java,
line 52
> > <https://reviews.apache.org/r/30466/diff/1/?file=842671#file842671line52>
> >
> >     This will make a new (empty) list each time it is called. Shouldn't the class
be hanging on to its currently known operator affinity list, even an empty one?

We are returning the empty list which is a static final in Collections. Most of the implementations
of GroupScan cache the result and don't recalculate everytime getOperatorAffinity() method
is called (I see some exceptions, I am going to address later as this problem is not introduced
by this change).


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MergingReceiverPOP.java,
line 58
> > <https://reviews.apache.org/r/30466/diff/1/?file=842680#file842680line58>
> >
> >     From these declarations, we can't tell what kind of List senders is; it might
be an ArrayList, or it might be a LinkedList. In the latter case, get(i) is expensive, because
it will iterate down the list to get to each item. Because of that, we should iterate over
that list instead, something like this:
> >     
> >     int i = 0;
> >     for(DrillbitEndpoint de : senders) {
> >       this.senders.put(i, de);
> >       ++i;
> >     }

The reason why are using get(i) is because we want to create a mapping of "i" -> DrillbitEndpoint.
DrillbitEndpoint assigned to minor fragment 3 is at index 3 in the list.


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MuxExchange.java,
line 131
> > <https://reviews.apache.org/r/30466/diff/1/?file=842681#file842681line131>
> >
> >     "this." isn't necessary here.
> >     
> >     I don't see any code that actually sets this variable.

Removed this. It is set in AbstractExchange.setupSenders/Receivers()


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java,
line 38
> > <https://reviews.apache.org/r/30466/diff/1/?file=842687#file842687line38>
> >
> >     Why don't we make this map final?

changed.


> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java,
line 46
> > <https://reviews.apache.org/r/30466/diff/1/?file=842687#file842687line46>
> >
> >     This code is the same as that in MergingReceiverPOP (which has one additional
different line at its end); should it be pulled up into AbstractReceiver?

Moved it to Abstract Receiver.


- Venki


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30466/#review70618
-----------------------------------------------------------


On Feb. 2, 2015, 7:21 p.m., Venki Korukanti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30466/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2015, 7:21 p.m.)
> 
> 
> Review request for drill, Chris Westin, Jacques Nadeau, and Steven Phillips.
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> In this patch LocalExchange contains only multiplexing exchange. Currently working on
demultiplexing exchange (there are few failures in executions and currently debugging those).
Sending this patch to get initial feedback.
> 
> Brief overview of changes:
> 1. Traverse the PRel tree after all optimizations. Whenever a HashToRandomExchangePrel
is encountered insert a MuxExchange before HashToRandomExchangePrel and DemuxExchange after
HashToRandomExchangePrel.
> 2. Parallelization changes: 
>    i) Traverse the physical operator tree and divide it into Fragments.
>    ii) Based on the affinity of the sending Exchange, set parallelization dependencies
between fragments. 
>    iii) Start parallelizing from the leaf fragments (fragment that have no other fragments
depending on them for parallelization info). Stats collection include collecting parallelization
info (minWidth, maxWidth, affinityMap) and cost.
> 3. Change the Receiver to accept set of (minorFragmentId, DrillbitEndpoint) as sender
list. This also involved few changes in DataCollector.
> 4. Change SingleSender to accept custom minorFragmentId instead of default minorFragmentId
of 0
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java df31f74

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractExchange.java
73280ea 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScan.java
5d0d9bf 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Exchange.java 7be7f20

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/GroupScan.java 23860a3

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/HasAffinity.java 52462db

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Receiver.java 0c67770

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Store.java 94411ea

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastExchange.java
73a1d20 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/HashToMergeExchange.java
f62d922 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/HashToRandomExchange.java
fac374b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MergingReceiverPOP.java
f5dca1a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MuxExchange.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/OrderedPartitionExchange.java
8e1526a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/Screen.java 58c8e29

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleMergeExchange.java
2914112 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleSender.java
4a11a51 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnionExchange.java
cfc21ac 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java
3a4dd0e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java
6db9f4a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Fragment.java ac63bde

>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/MakeFragmentsVisitor.java
8756e5b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java
961b603 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/ParallelizationInfo.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/PlanningSet.java
8cc6c85 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java
434cdd4 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Stats.java eda364b

>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/StatsCollector.java
41ff678 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java 86b395e

>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/MuxExchangePrel.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java
faa8546 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java
79603eb 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
f20627d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/mock/MockStorePOP.java 4c12d57

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/AbstractDataCollector.java
c83106c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/DataCollector.java dc016be

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/IncomingBuffers.java
b0206f7 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/MergingCollector.java
ce14260 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/PartitionedCollector.java
5190d84 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java b33042b

>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 6349b76

> 
> Diff: https://reviews.apache.org/r/30466/diff/
> 
> 
> Testing
> -------
> 
> Ran Functional and TPCH SF100 parquet verification tests.
> 
> 
> Thanks,
> 
> Venki Korukanti
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message