----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34541/#review84943 -----------------------------------------------------------
Steven knows much more about this area of the code than I do, so my comments are mostly stylistic, with one exception. exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java <https://reviews.apache.org/r/34541/#comment136395> Can we make this final? exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java <https://reviews.apache.org/r/34541/#comment136399> Nothing else in this file is synchronized, so what is this protecting? exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java <https://reviews.apache.org/r/34541/#comment136401> final exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java <https://reviews.apache.org/r/34541/#comment136402> Can we indent this a little better so it's easier to see the nested select separately from the rest of the query, as well as the order by and group by clauses? It might help to condense the select lists onto a single line. For example select ....., more columns, if necessary from ... more of that, if necessary where ... group by ... order by ... limit ... exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java <https://reviews.apache.org/r/34541/#comment136403> Spaces around the + operator, please. exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java <https://reviews.apache.org/r/34541/#comment136404> Can we move this to BaseTestQuery and make it protected? Other tests could use this. - Chris Westin On May 21, 2015, 12:34 p.m., abdelhakim deneche wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34541/ > ----------------------------------------------------------- > > (Updated May 21, 2015, 12:34 p.m.) > > > Review request for drill, Chris Westin and Jacques Nadeau. > > > Bugs: DRILL-3147 > https://issues.apache.org/jira/browse/DRILL-3147 > > > Repository: drill-git > > > Description > ------- > > - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized > - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because > it requires a large dataset > > > Diffs > ----- > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java > 80d2d6e > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java > 11b6cc8 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java > b770a33 > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/34541/diff/ > > > Testing > ------- > > still need to run the tests! > > > Thanks, > > abdelhakim deneche > >
