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 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes memory leak
Date Fri, 22 May 2015 18:15:56 GMT

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


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