drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From julienledem <...@git.apache.org>
Subject [GitHub] drill pull request: DRILL-4134: Allocator updates
Date Tue, 01 Dec 2015 00:47:10 GMT
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.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message