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