drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From paul-rogers <...@git.apache.org>
Subject [GitHub] drill pull request #1073: DRILL-5967: Fixed memory leak in OrderedPartitionS...
Date Wed, 20 Dec 2017 23:00:14 GMT
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?


---

Mime
View raw message