Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1073#discussion_r158158542
--- 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 --
Under what scenario would we *not* close the incoming? This fix introduces
two paths: one that closes, one that does not. Is the non-closing path needed?
More to the point, do we have unit tests that verify both paths?
Can we simplify this and just define that this operator will close the
incoming?
Actually, let's take a step back. This fix is based on a false premise (but
one a mistake that is easy to make.) This fix assumes that a parent batch
(operator) is responsible for closing its child. But, that is not how the Drill
protocol works.
Instead, the fragment executor maintains the operator tree. Upon exit, it
walks the tree and closes all its children in order from top down.
So, the true source of this bug is that either a) the fragment exec is
failing to do the close, b) that the incoming is not releasing memory on close,
or c) that some close operation reallocates memory that a previous close tried
to release.
Let's suppose the issue is a: that the fragment exec does not manage the
incoming. Let's fix that. Similar with the other cases.
Can you maybe do a bit more research to understand the full scope of the
issue?
---
