Re: 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/ --- (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
--- 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
--- 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
