> 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 > >
