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 #1058: DRILL-6002: Avoid memory copy from direct buffer t...
Date Sat, 02 Dec 2017 00:26:56 GMT
Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1058#discussion_r154478289
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/BatchGroup.java
---
    @@ -249,7 +247,7 @@ public void close() throws IOException {
             ex = e;
           }
           try {
    -        closeOutputStream();
    +        closeWriter();
    --- End diff --
    
    Just as a matter of record, this abstraction is rather awkward. It tries to be three things:
1) a writer for a spill file, 2) a reader for a spill file and 3) the tombstone for the spill
file.
    
    It would be much better to have separate reader and writer classes that come and go, with
this class just representing the spill file over its lifetime. There was, however, no easy
way to make that change and preserve existing code. Since I'd already changed more code than
I really wanted, I left well enough alone. (See the "unmanaged" version for the original code,
which was even more murky.)
    
    Now that you've modified the Writer to be the batch writer, I wonder if we should revisit
the issue rather than preserving the old, muddy, semantics of this class.


---

Mime
View raw message