Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/1073#discussion_r158577033
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java
---
@@ -341,6 +351,10 @@ public void close() throws Exception {
updateAggregateStats();
partitioner.clear();
}
+
+ if (closeIncoming) {
+ ((CloseableRecordBatch) incoming).close();
+ }
--- End diff --
Hi Paul, I understand the FragmentExecutor closes all the operators.
However, the FragmentExecutor is only aware of the operators in the tree that
it has a reference too. In the case of the OrderedPartitionSenderCreator, an
upstream record batch is wrapped in an OrderedPartitionRecordBatch. Since the
OrderedPartitionRecordBatch is instantiated in the creator the FragmentExecutor
does not have a reference to it. So when the FragmentExecutor closes the
operators it closes the original operator, but not not the wrapping
OrderedPartitionRecordBatch. This is an issue since the
OrderedPartitionRecordBatch allocates VectorContainers which are
consequentially never released. This is clearly the source of the memory leak
and the fix was verified by QA.
There are two possible Fixes.
A) We change the Creators in some way to communicate to the
FragmentExecutor that they have wrapped an operator, so the FragmentExecutor
can close the wrapped operator instead of the original operator. My impression
is that this will evolve into a fairly large change.
B) Or we take a less invasive approach and simply tell the
PartitionSenderRootExec whether to close the wrapped operator (This is what I
did).
I agree approach B is not that great and that approach A is cleaner. But
taking approach A is a more intensive project, and I was not keen on having
another simple change explode into another 100 file PR. It would be easier to
make that change after we've made more improvements to Drill unit testing.
Let me know what you think. I am not against taking approach A, but then
we'll have to talk with Pritesh about fitting that project in.
---
