----------------------------------------------------------- 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 > >
