> On April 8, 2015, 7:38 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/SendingAccountor.java, > > line 46 > > <https://reviews.apache.org/r/32949/diff/1-2/?file=920359#file920359line46> > > > > 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.
I am removing this change and doing a separate patch. - Steven ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32949/#review79414 ----------------------------------------------------------- On April 8, 2015, 7: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, 7: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 > >
