drill-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (DRILL-4134) Incorporate remaining patches from DRILL-1942 Allocator refactor
Date Tue, 01 Dec 2015 00:47:10 GMT

    [ https://issues.apache.org/jira/browse/DRILL-4134?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15032819#comment-15032819
] 

ASF GitHub Bot commented on DRILL-4134:
---------------------------------------

Github user julienledem commented on a diff in the pull request:

    https://github.com/apache/drill/pull/283#discussion_r46226910
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java
---
    @@ -167,11 +169,29 @@ private void send(final FragmentRecordBatch fragmentBatch, final
DrillBuf body,
         }
     
         final BufferAllocator allocator = manager.getFragmentContext().getAllocator();
    -    final Pointer<DrillBuf> out = new Pointer<>();
    -
         final boolean withinMemoryEnvelope;
    -
    -    withinMemoryEnvelope = allocator.takeOwnership(body, out);
    +    final DrillBuf transferredBuffer;
    +    try {
    +      TransferResult result = body.transferOwnership(allocator);
    +      withinMemoryEnvelope = result.allocationFit;
    +      transferredBuffer = result.buffer;
    +    } catch(final AllocatorClosedException e) {
    +      /*
    +       * It can happen that between the time we get the fragment manager and we
    +       * try to transfer this buffer to it, the fragment may have been cancelled
    +       * and closed. When that happens, the allocator will be closed when we
    +       * attempt this. That just means we can drop this data on the floor, since
    +       * the receiver no longer exists (and no longer needs it).
    +       *
    +       * Note that checking manager.isCancelled() before we attempt this isn't enough,
    +       * because of timing: it may still be cancelled between that check and
    +       * the attempt to do the memory transfer. To double check ourselves, we
    +       * do check manager.isCancelled() here, after the fact; it shouldn't
    +       * change again after its allocator has been closed.
    +       */
    +      assert manager.isCancelled();
    --- End diff --
    
    I would use a regular exception here:
    ```
    if (!manager.isCancelled()) {
       throw new ...
    }
    ```
    that way you can have a better message than just AssertionError and chain e to the exception.


> Incorporate remaining patches from DRILL-1942 Allocator refactor
> ----------------------------------------------------------------
>
>                 Key: DRILL-4134
>                 URL: https://issues.apache.org/jira/browse/DRILL-4134
>             Project: Apache Drill
>          Issue Type: Sub-task
>          Components: Execution - Flow
>            Reporter: Jacques Nadeau
>            Assignee: Jacques Nadeau
>             Fix For: 1.4.0
>
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message