You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by abdelhakim deneche <ad...@gmail.com> on 2015/05/19 00:10:28 UTC
Review Request 34374: DRILL-3133: MergingRecordBatch can leak memory
if query is canceled before batches in rawBatches were loaded
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34374/
-----------------------------------------------------------
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
-------
unit test and tpch100 are passing
Thanks,
abdelhakim deneche
Re: Review Request 34374: DRILL-3133: MergingRecordBatch can leak
memory if query is canceled before batches in rawBatches were loaded
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34374/
-----------------------------------------------------------
(Updated July 10, 2015, 7:33 p.m.)
Review request for drill and Steven Phillips.
Changes
-------
addressing review comments.
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 (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 3ca11f1
Diff: https://reviews.apache.org/r/34374/diff/
Testing
-------
all unit tests are passing along with functional and tpch100
Thanks,
abdelhakim deneche
Re: Review Request 34374: DRILL-3133: MergingRecordBatch can leak
memory if query is canceled before batches in rawBatches were loaded
Posted by Steven Phillips <sp...@maprtech.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 34374: DRILL-3133: MergingRecordBatch can leak
memory if query is canceled before batches in rawBatches were loaded
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-------
all unit tests are passing along with functional and tpch100
Thanks,
abdelhakim deneche