You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Chris Westin <ch...@gmail.com> on 2015/05/12 07:58:22 UTC
Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so
far in DRILL-1942 to 1.0.0
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/
-----------------------------------------------------------
(Updated May 11, 2015, 10:58 p.m.)
Review request for drill and Jacques Nadeau.
Bugs: DRILL-3033
https://issues.apache.org/jira/browse/DRILL-3033
Repository: drill-git
Description
-------
Fixes some missing buffer retains() and missing vector clears().
Diffs
-----
exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810
exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb
exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 659d99b
exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java 8e2ce96
exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java c233ac5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java 369c0ec
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java c3e70f5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java 0430f1b
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java 1187bd6
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 5d990f0
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java ca6d83c
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java 8748aaf
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java e559ece
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java 3159811
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 9b97e1c
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java f7786b7
exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 308a8bc
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java ae5fad5
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java 78846dc
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java b615b66
exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 2b1dff0
Diff: https://reviews.apache.org/r/34075/diff/
Testing (updated)
-------
mvn install
precommit suite - has one failure, will try again to see what happens because:
Execution Failures:
/root/private-sql-hadoop-test/framework/resources/Precommit/Functional/complex/parquet/complex274.q
Query:
null
Failed with exception
java.sql.SQLException: SYSTEM ERROR: org.apache.drill.common.exceptions.ExecutionSetupException: Two processes tried to change a plugin at the same time.
[Error Id: 78607318-d1fb-4d8d-bb60-c79d6d1f978c on drillats3.qa.lab:31010]
at org.apache.drill.jdbc.DrillCursor.next(DrillCursor.java:161)
at org.apache.drill.jdbc.impl.DrillResultSetImpl.execute(DrillResultSetImpl.java:167)
at org.apache.drill.jdbc.impl.DrillResultSetImpl.execute(DrillResultSetImpl.java:56)
at net.hydromatic.avatica.AvaticaConnection.executeQueryInternal(AvaticaConnection.java:404)
at net.hydromatic.avatica.AvaticaStatement.executeQueryInternal(AvaticaStatement.java:351)
at net.hydromatic.avatica.AvaticaStatement.executeQuery(AvaticaStatement.java:78)
at org.apache.drill.jdbc.impl.DrillStatementImpl.executeQuery(DrillStatementImpl.java:89)
at org.apache.drill.test.framework.DrillTestJdbc.executeSetupQuery(DrillTestJdbc.java:127)
Thanks,
Chris Westin
Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so
far in DRILL-1942 to 1.0.0
Posted by Chris Westin <ch...@gmail.com>.
> On May 12, 2015, 9:13 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java, line 218
> > <https://reviews.apache.org/r/34075/diff/1/?file=956037#file956037line218>
> >
> > Shouldn't this and above be target.clear() rather than reaching inside?
I go back and forth on it. As I've mentioned before, I'd like to see us move to not treating the empty buffer specially. One of the changes there is to have a close() that is different from clear(), which does a data.release(1), and then sets data to null. I tried that out, and found that some vectors are resurrected after being closed, and are used again. So I started going with the check for null.
I can change it to clear() if you like. What's your preference?
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/#review83413
-----------------------------------------------------------
On May 12, 2015, 8:22 a.m., Chris Westin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34075/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 8:22 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-3033
> https://issues.apache.org/jira/browse/DRILL-3033
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Fixes some missing buffer retains() and missing vector clears().
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 659d99b
> exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java 8e2ce96
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java c233ac5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java 369c0ec
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java c3e70f5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java 0430f1b
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java 1187bd6
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 5d990f0
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java ca6d83c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java 8748aaf
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java e559ece
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java 3159811
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 9b97e1c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java f7786b7
> exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 308a8bc
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java ae5fad5
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java 78846dc
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java b615b66
> exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 2b1dff0
>
> Diff: https://reviews.apache.org/r/34075/diff/
>
>
> Testing
> -------
>
> mvn install
> precommit suite
>
>
> Thanks,
>
> Chris Westin
>
>
Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so
far in DRILL-1942 to 1.0.0
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/#review83413
-----------------------------------------------------------
One quick comment, otherwise looks good.
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java
<https://reviews.apache.org/r/34075/#comment134404>
Shouldn't this and above be target.clear() rather than reaching inside?
- Jacques Nadeau
On May 12, 2015, 3:22 p.m., Chris Westin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34075/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 3:22 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-3033
> https://issues.apache.org/jira/browse/DRILL-3033
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Fixes some missing buffer retains() and missing vector clears().
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810
> exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb
> exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 659d99b
> exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java 8e2ce96
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java c233ac5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java 369c0ec
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java c3e70f5
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java 0430f1b
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java 1187bd6
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 5d990f0
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java ca6d83c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java 8748aaf
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java e559ece
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java 3159811
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 9b97e1c
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java f7786b7
> exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 308a8bc
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java ae5fad5
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java 78846dc
> exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java b615b66
> exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 2b1dff0
>
> Diff: https://reviews.apache.org/r/34075/diff/
>
>
> Testing
> -------
>
> mvn install
> precommit suite
>
>
> Thanks,
>
> Chris Westin
>
>
Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so
far in DRILL-1942 to 1.0.0
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/
-----------------------------------------------------------
(Updated May 12, 2015, 11:57 a.m.)
Review request for drill and Jacques Nadeau.
Changes
-------
Addressed review request.
Bugs: DRILL-3033
https://issues.apache.org/jira/browse/DRILL-3033
Repository: drill-git
Description
-------
Fixes some missing buffer retains() and missing vector clears().
Diffs (updated)
-----
exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810
exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb
exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 659d99b
exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java 8e2ce96
exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java c233ac5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java 369c0ec
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java c3e70f5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java 0430f1b
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java 1187bd6
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 5d990f0
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java ca6d83c
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java 8748aaf
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java e559ece
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java 3159811
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 9b97e1c
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java f7786b7
exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 308a8bc
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java ae5fad5
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java 78846dc
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java b615b66
exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 2b1dff0
Diff: https://reviews.apache.org/r/34075/diff/
Testing
-------
mvn install
precommit suite
Thanks,
Chris Westin
Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so
far in DRILL-1942 to 1.0.0
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/
-----------------------------------------------------------
(Updated May 12, 2015, 8:22 a.m.)
Review request for drill and Jacques Nadeau.
Bugs: DRILL-3033
https://issues.apache.org/jira/browse/DRILL-3033
Repository: drill-git
Description
-------
Fixes some missing buffer retains() and missing vector clears().
Diffs
-----
exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810
exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb
exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 659d99b
exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java 8e2ce96
exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java c233ac5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java 369c0ec
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java c3e70f5
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java 0430f1b
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java 1187bd6
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 5d990f0
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java ca6d83c
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java 8748aaf
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java e559ece
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java 3159811
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java 9b97e1c
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java f7786b7
exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 308a8bc
exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java ae5fad5
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java 78846dc
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java b615b66
exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java 2b1dff0
Diff: https://reviews.apache.org/r/34075/diff/
Testing (updated)
-------
mvn install
precommit suite
Thanks,
Chris Westin