drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yuliya Feldman" <yufeld...@yahoo.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 07:13:50 GMT


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> >

Thank you very much Chris for review - see my comments to your comments


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java, line
69
> > <https://reviews.apache.org/r/31107/diff/1/?file=866190#file866190line69>
> >
> >     Why not call this(original, false) to avoid duplicating the code?

oops - forgot to remove this method - will do


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorStats.java, line
115
> > <https://reviews.apache.org/r/31107/diff/1/?file=866190#file866190line115>
> >
> >     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.

OK


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java,
line 32
> > <https://reviews.apache.org/r/31107/diff/1/?file=866191#file866191line32>
> >
> >     I'd make these final too, while you're here.

OK


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java,
line 286
> > <https://reviews.apache.org/r/31107/diff/1/?file=866192#file866192line286>
> >
> >     Reuse the id variable from above (which looks like it should be made final).

yes - definitely


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java,
line 46
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line46>
> >
> >     Could this be final?

Don't see a reason why not, but will double check


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java,
line 164
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line164>
> >
> >     Call Thread.currentThread() once at the beginning, and then reuse.

It is a beginnging - as I am within callable. I am trying to get the name of the thread that
executes "Callable" to include name of it's parent thread (tname).
some parts can be reused but not the full name


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java,
line 166
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line166>
> >
> >     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.

There not much point in restoring original state, as those threads are only used to call "callable"
and so each next thread will rename it to something it needs every time


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java,
line 178
> > <https://reviews.apache.org/r/31107/diff/1/?file=866194#file866194line178>
> >
> >     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?

Since method that evenually calls this one throws only IOException I am throwing it. Will
add message


> On Feb. 19, 2015, 5:12 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java,
line 47
> > <https://reviews.apache.org/r/31107/diff/1/?file=866191#file866191line47>
> >
> >     This should do something other than swallow InterruptedException. See http://www.ibm.com/developerworks/library/j-jtp05236/
.

Not that I touched this code, I need to see what should be done here.


- Yuliya


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


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