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

    https://github.com/apache/drill/pull/811#discussion_r114465904
  
    --- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 
---
    @@ -143,20 +104,18 @@ public void close() {
         }
         logger.debug("Closing context for {}", popConfig != null ? 
popConfig.getClass().getName() : null);
     
    -    manager.close();
    -
    -    if (allocator != null) {
    -      allocator.close();
    -    }
    -
    -    if (fs != null) {
    -      try {
    -        fs.close();
    -      } catch (IOException e) {
    -        throw new DrillRuntimeException(e);
    +    closed = true;
    --- End diff --
    
    To see this better, view the full source. You'll see these lines:
    ```
      public void close() {
        if (closed) {
          logger.debug("Attempted to close Operator context for {}, but context 
is already closed", popConfig != null ? popConfig.getClass().getName() : null);
          return;
        }
        logger.debug("Closing context for {}", popConfig != null ? 
popConfig.getClass().getName() : null);
        closed = true;
    ```
    
    Moving the line up makes it clearer that the only purpose of this flag is 
to enforce close-once semantics. (The check is original code.)
    
    Before, this function did not call super.close(), which is necessary to 
close the allocator (which used to be here.)
    
    All closes are wrapped in exceptions. If super.close() fails, we still 
close the file system.


---
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 [email protected] or file a JIRA ticket
with INFRA.
---

Reply via email to