drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 32949: DRILL-2718: Handle counting and status of sent batches by FragmentContext
Date Wed, 08 Apr 2015 19:38:58 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/#review79414
-----------------------------------------------------------



exec/java-exec/src/main/java/org/apache/drill/exec/ops/SendingAccountor.java
<https://reviews.apache.org/r/32949/#comment128743>

    This looks wrong. You're always waiting for all of batchesSent, even if you've waited
for some of them before (since you never reset it now). You need something like
    
    int waitForBatches;
    while((waitForBatches = batchesSent.get() - finishedBatches.get()) != 0) {
      ...
      }
    
    I'm also a little worried that this can cause problems because the increment of finishedBatches
and the release aren't synchronized/atomic. It seems like finishedBatches could be incremented,
and then the running thread is suspended (unscheduled). Then another thread calls waitForSendComplete();
batchesSent - finishedBatches could be zero, even though the wait.release() hasn't been called.
So we don't wait for that when it seems like we should. That might not cause a problem, but
it's a little weird. Maybe there are other scenarios where it can be a problem.
    
    When I made the fix not to have lost updates (check the history for the recent change
to getAndSet(0)), I tried synchronizing the other methods, and that caused a deadlock, because
one thread couldn't enter decrement because another one was in waitForSendComplete() at the
wait. So just synchronizing everything doesn't fix this.
    
    What was the purpose of this change? The last version seemed to be correct.



exec/java-exec/src/main/java/org/apache/drill/exec/ops/SendingAccountor.java
<https://reviews.apache.org/r/32949/#comment128744>

    I think we still need some kind of TODO here, otherwise whoever called waitForSendComplete
is going to go on their merry way, assuming everything is done, and releasing stuff, even
though it might still be getting sent. We may need to throw a different exception here to
cause things to be cleaned up carefully. Please restore the TODO.


- Chris Westin


On April 8, 2015, 12:10 p.m., Steven Phillips wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32949/
> -----------------------------------------------------------
> 
> (Updated April 8, 2015, 12:10 p.m.)
> 
> 
> Review request for drill, Chris Westin and Jacques Nadeau.
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> Creates wrapper classes FragmentDataTunnel and FragmentUserDataTunnel which wrap
> the DataTunnel and UserClientConnection, respectively, allowing us to use DataTunnels
> and UserClientConnections from a global pool, but track pending batches and send status
> at the FragmentContext level.
> 
> Consolidates the various StatusListener implementations used by the various senders and
> instead uses just one implementation.
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 18b93e9

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/SendingAccountor.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
a00df9d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
8038527 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java
21fc800 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java
1ef7bbd 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java
d17fdd4 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java
6a73cdd 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java
9d6e98f 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java
33d6f95 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java
5e21878 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 8022c95

> 
> Diff: https://reviews.apache.org/r/32949/diff/
> 
> 
> Testing
> -------
> 
> No new functionality, so no new tests. Current tests all pass.
> 
> 
> Thanks,
> 
> Steven Phillips
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message