Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1058#discussion_r154476440
  
    --- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorSerializer.java 
---
    @@ -40,20 +52,18 @@
        */
     
       public static class Writer {
    +    static final MetricRegistry metrics = DrillMetrics.getRegistry();
    +    static final String WRITER_TIMER = 
MetricRegistry.name(VectorAccessibleSerializable.class, "writerTime");
     
    -    private final OutputStream stream;
    -    private final BufferAllocator allocator;
    -    private boolean retain;
    +    private final SpillSet spillSet;
    +    private final WritableByteChannel channel;
    +    private final OutputStream output;
         private long timeNs;
     
    -    public Writer(BufferAllocator allocator, OutputStream stream) {
    -      this.allocator = allocator;
    -      this.stream = stream;
    -    }
    -
    -    public Writer retain() {
    -      retain = true;
    -      return this;
    +    private Writer(SpillSet spillSet, String path) throws IOException {
    --- End diff --
    
    Before this class was independent of the `SpillSet`. Now, it can be used 
only with that class. Not sure if this is an improvement.
    
    The only thing it seems we need from `SpillSet` is to update the write byte 
count. Perhaps define an interface here with just the needed methods. Then, let 
`SpillSet` implement that interface. That gets what you need without 
introducing extra dependencies.


---

Reply via email to