drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 31107: Ability to make PartitionSender multithreaded - useful in case of LocalExchange being enabled, as it allows to deal with high volume of incoming data
Date Fri, 20 Feb 2015 01:12:05 GMT

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



exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java
<https://reviews.apache.org/r/31107/#comment119501>

    Why not call this(original, false) to avoid duplicating the code?



exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java
<https://reviews.apache.org/r/31107/#comment119502>

    how about
    
    final IntLongOpenHashmap fromMetrics = from.longMetrics;
    final long[] fromValues = fromMetrics.values;
    for(int i : fromMetrics.keys) {
      final long value = fromValues[i];
      longMetrics.putOrAdd(i, value, value);
    } 
    
    This avoids multiple evaluation of the member accesses on every pass of the loop, and
multiple bounds checks within the loop body.
    
    Similar improvements can be made to the doubleMetrics loop below.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java
<https://reviews.apache.org/r/31107/#comment119503>

    I'd make these final too, while you're here.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java
<https://reviews.apache.org/r/31107/#comment119504>

    This should do something other than swallow InterruptedException. See http://www.ibm.com/developerworks/library/j-jtp05236/
.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java
<https://reviews.apache.org/r/31107/#comment119506>

    Reuse the id variable from above (which looks like it should be made final).



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119507>

    Could this be final?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119508>

    Call part.getStats() once at the beginning, and reuse the local throughout.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119510>

    part.getStats() once.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119511>

    Call Thread.currentThread() once at the beginning, and then reuse.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119513>

    Would it make sense to restore the original thread name here? If so, wrap the iface.execute()
in a try and restore the name in a finally.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
<https://reviews.apache.org/r/31107/#comment119514>

    Is IOException the best choice here? Would RuntimeException make more sense? Either way,
would a message be helpful in case there are problems? How would a user know what to do if
this exception fired?


- Chris Westin


On Feb. 17, 2015, 12:31 a.m., Yuliya Feldman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31107/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2015, 12:31 a.m.)
> 
> 
> Review request for drill, Chris Westin, Jacques Nadeau, Steven Phillips, and Venki Korukanti.
> 
> 
> Bugs: DRILL-2210
>     https://issues.apache.org/jira/browse/DRILL-2210
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> In addition to description
> 
> Fixed few classes that did not handle multithreading well
> Added/Changed some Stats behavior to allow stats merge from multiple threads, since again
this class is not suitable to be used in multithreaded environment
> Introduced new decorator class to handle multi thrteading (or not)  to minimize changes
to ParitionSenderRootExec class
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java 0e9da0e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java
7af7b65 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java
f09acaa 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java
5ed9c39 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java
4292c09 
>   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/server/options/SystemOptionManager.java
aa0a5ad 
> 
> Diff: https://reviews.apache.org/r/31107/diff/
> 
> 
> Testing
> -------
> 
> Still need to provide Unit Tests.
> 
> Functional tests are passing
> 
> Performance tests were run and look promising for some queries
> 
> 
> Thanks,
> 
> Yuliya Feldman
> 
>


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