drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steven Phillips" <sphill...@maprtech.com>
Subject Re: Review Request 34374: DRILL-3133: MergingRecordBatch can leak memory if query is canceled before batches in rawBatches were loaded
Date Wed, 08 Jul 2015 06:17:13 GMT

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

Ship it!



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
(line 307)
<https://reviews.apache.org/r/34374/#comment144026>

    We should remove this if block altogether. It's clearly not doing anything.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
(line 339)
<https://reviews.apache.org/r/34374/#comment144025>

    It probably makes sense to release the batch there, but it's not necessary because the
RecordBatchLoader releases the buffers when it loads the new ones, or when close() is called.
So there is no memory leak here.


- Steven Phillips


On May 28, 2015, 11:54 a.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34374/
> -----------------------------------------------------------
> 
> (Updated May 28, 2015, 11:54 a.m.)
> 
> 
> Review request for drill and Steven Phillips.
> 
> 
> Bugs: DRILL-3133
>     https://issues.apache.org/jira/browse/DRILL-3133
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> MergingRecordBatch stores batches in an array list before loading them with RecordBatchLoader.
If the query is canceled before all received batches are loaded, some of the batches won't
be cleaned up.
> 
> lines 307 and 339 contain questions to the reviewers. I will update the patch accordingly
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
baf9bda 
> 
> Diff: https://reviews.apache.org/r/34374/diff/
> 
> 
> Testing
> -------
> 
> all unit tests are passing along with functional and tpch100
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


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