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

    https://github.com/apache/drill/pull/666#discussion_r90690234
  
    --- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WriterRecordBatch.java
 ---
    @@ -174,10 +174,15 @@ protected void setupNewSchema() throws IOException {
         schema = container.getSchema();
       }
     
    +  /** Partially written data will be removed, otherwise clean up will be 
performed. */
       private void closeWriter() {
         if (recordWriter != null) {
           try {
    -        recordWriter.cleanup();
    +        if (processed) {
    +          recordWriter.cleanup();
    +        } else {
    +          recordWriter.abort();
    +        }
    --- End diff --
    
    Can these fail? If so, should we set the recordWriter to null in the 
finally section so that the writer is marked as closed even in the failed-close 
case?
    
    Also, we mark the context as failed below. Must the caller check this 
status and propagate the error up the call stack? Does this happen? Should this 
method throw an exception instead to force the attention of the caller?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to