You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Steven Phillips <sp...@maprtech.com> on 2015/04/08 02:57:13 UTC
Review Request 32949: DRILL-2718: Handle counting and status of sent
batches by FragmentContext
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/
-----------------------------------------------------------
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
Re: Review Request 32949: DRILL-2718: Handle counting and status of
sent batches by FragmentContext
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/#review79397
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128731>
Can't this be final?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128707>
Is it really right to return a new one each time, rather than finding and returning a single one (and creating it only the first time)?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128713>
Can this be final?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128712>
fail() should worry about doing something or ignoring calls according to the current state, rather than depending on the caller to worry about it. In this case, fail() would probably like to add the exception as a suppressed exception under a pre-existing fail, so it should still be called (subject to having any necessary adjustments to handle that).
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128714>
Can this be final?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128726>
Does this really have to be an inner class? The only thing it references is FragmentContext.sendingAccountor. I would rather create an "AccountingDataTunnel" that takes the DataTunnel and a SendingAccountor as its constructor arguments. We might find other uses for that besides here. I'd rather keep FragmentContext as uncluttered as possible.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128715>
final
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
<https://reviews.apache.org/r/32949/#comment128727>
Same comment as FragmentDataTunnel re this being an inner class. This could be "AccountingUserConnection" or something like that.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
<https://reviews.apache.org/r/32949/#comment128720>
Too many contexts; can we please rename this to fragmentContext?
Also, can't all three of these (stats, oContext, fragmentContext) be final? Remove the initializations to null, because it looks like they are all initialized in the constructors.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
<https://reviews.apache.org/r/32949/#comment128721>
Why is this (and incoming, and context) not private?
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java
<https://reviews.apache.org/r/32949/#comment128722>
Wouldn't it make more sense to call super.stop() after the logger message? After all, that is part of the stopping process.
- 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
>
>
Re: Review Request 32949: DRILL-2718: Handle counting and status of
sent batches by FragmentContext
Posted by Steven Phillips <sp...@maprtech.com>.
> 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
>
>
Re: Review Request 32949: DRILL-2718: Handle counting and status of
sent batches by FragmentContext
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 32949: Patch for DRILL-2718
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/#review79911
-----------------------------------------------------------
Ship it!
Ship it (non-binding).
- Chris Westin
On April 10, 2015, 11:47 p.m., Steven Phillips wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32949/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 11:47 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-2718
> https://issues.apache.org/jira/browse/DRILL-2718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2718: Move counting and tracking of sent batches to FragmentContext
>
> 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/AccountingDataTunnel.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/Consumer.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 18b93e90853bb18971ad7a21f9a6d4bd9853c1bb
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/StatusHandler.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java a00df9d27a981fd6b85d3fd2a272a00c4aba80d6
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 8038527a50a1e1f6cdb6dce68b7d5438869f928f
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 21fc800cc9855ee2da309934a4422dd37644f4bf
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java 1ef7bbd4691f0ced794e0f8d1dc3921b60044931
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java d17fdd4ba8bc3037a26cfcc791aeb18f3a112cfe
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java 6a73cdd6b4124f7410e3eb3cfaf9c7a3e17fed8d
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 9d6e98ff9ad59450bbd274ea99417a8f1acf5915
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 33d6f9560133c4d8f97c1d214c9fa007216197ea
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java 5e21878d450825731317742638ef1e6d3f3cee30
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 8022c95821e3bac6c5d0f249cf52c85af29fd1e8
>
> Diff: https://reviews.apache.org/r/32949/diff/
>
>
> Testing
> -------
>
> No new functionality, so no new tests. Current tests all pass.
>
>
> Thanks,
>
> Steven Phillips
>
>
Re: Review Request 32949: Patch for DRILL-2718
Posted by Steven Phillips <sp...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/
-----------------------------------------------------------
(Updated April 11, 2015, 6:47 a.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Bugs: DRILL-2718
https://issues.apache.org/jira/browse/DRILL-2718
Repository: drill-git
Description
-------
DRILL-2718: Move counting and tracking of sent batches to FragmentContext
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 (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingDataTunnel.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/Consumer.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 18b93e90853bb18971ad7a21f9a6d4bd9853c1bb
exec/java-exec/src/main/java/org/apache/drill/exec/ops/StatusHandler.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java a00df9d27a981fd6b85d3fd2a272a00c4aba80d6
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 8038527a50a1e1f6cdb6dce68b7d5438869f928f
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 21fc800cc9855ee2da309934a4422dd37644f4bf
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java 1ef7bbd4691f0ced794e0f8d1dc3921b60044931
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java d17fdd4ba8bc3037a26cfcc791aeb18f3a112cfe
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java 6a73cdd6b4124f7410e3eb3cfaf9c7a3e17fed8d
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 9d6e98ff9ad59450bbd274ea99417a8f1acf5915
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 33d6f9560133c4d8f97c1d214c9fa007216197ea
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java 5e21878d450825731317742638ef1e6d3f3cee30
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 8022c95821e3bac6c5d0f249cf52c85af29fd1e8
Diff: https://reviews.apache.org/r/32949/diff/
Testing
-------
No new functionality, so no new tests. Current tests all pass.
Thanks,
Steven Phillips
Re: Review Request 32949: Patch for DRILL-2718
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/#review79747
-----------------------------------------------------------
Just a few minor comments on comments and strings, so if you agree with those and apply them, Ship it (non-binding).
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingDataTunnel.java
<https://reviews.apache.org/r/32949/#comment129278>
Just "... that tracks the status ..." ?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/StatusHandler.java
<https://reviews.apache.org/r/32949/#comment129281>
It might not be a fragment, which is why I removed the second sentence from the sample I sent you earlier.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/Consumer.java
<https://reviews.apache.org/r/32949/#comment129280>
// TODO replace this when we get JDK8, which includes this
- Chris Westin
On April 10, 2015, 1:20 p.m., Steven Phillips wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32949/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 1:20 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Bugs: DRILL-2718
> https://issues.apache.org/jira/browse/DRILL-2718
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2718: Move counting and tracking of sent batches to FragmentContext
>
> 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/AccountingDataTunnel.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/Consumer.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 18b93e90853bb18971ad7a21f9a6d4bd9853c1bb
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/StatusHandler.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java a00df9d27a981fd6b85d3fd2a272a00c4aba80d6
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 8038527a50a1e1f6cdb6dce68b7d5438869f928f
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 21fc800cc9855ee2da309934a4422dd37644f4bf
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java 1ef7bbd4691f0ced794e0f8d1dc3921b60044931
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java d17fdd4ba8bc3037a26cfcc791aeb18f3a112cfe
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java 6a73cdd6b4124f7410e3eb3cfaf9c7a3e17fed8d
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 9d6e98ff9ad59450bbd274ea99417a8f1acf5915
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 33d6f9560133c4d8f97c1d214c9fa007216197ea
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java 5e21878d450825731317742638ef1e6d3f3cee30
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 8022c95821e3bac6c5d0f249cf52c85af29fd1e8
>
> Diff: https://reviews.apache.org/r/32949/diff/
>
>
> Testing
> -------
>
> No new functionality, so no new tests. Current tests all pass.
>
>
> Thanks,
>
> Steven Phillips
>
>
Re: Review Request 32949: Patch for DRILL-2718
Posted by Steven Phillips <sp...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/
-----------------------------------------------------------
(Updated April 10, 2015, 8:20 p.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Bugs: DRILL-2718
https://issues.apache.org/jira/browse/DRILL-2718
Repository: drill-git
Description
-------
DRILL-2718: Move counting and tracking of sent batches to FragmentContext
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 (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingDataTunnel.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/Consumer.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 18b93e90853bb18971ad7a21f9a6d4bd9853c1bb
exec/java-exec/src/main/java/org/apache/drill/exec/ops/StatusHandler.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java a00df9d27a981fd6b85d3fd2a272a00c4aba80d6
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 8038527a50a1e1f6cdb6dce68b7d5438869f928f
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 21fc800cc9855ee2da309934a4422dd37644f4bf
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java 1ef7bbd4691f0ced794e0f8d1dc3921b60044931
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java d17fdd4ba8bc3037a26cfcc791aeb18f3a112cfe
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java 6a73cdd6b4124f7410e3eb3cfaf9c7a3e17fed8d
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 9d6e98ff9ad59450bbd274ea99417a8f1acf5915
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 33d6f9560133c4d8f97c1d214c9fa007216197ea
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java 5e21878d450825731317742638ef1e6d3f3cee30
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 8022c95821e3bac6c5d0f249cf52c85af29fd1e8
Diff: https://reviews.apache.org/r/32949/diff/
Testing
-------
No new functionality, so no new tests. Current tests all pass.
Thanks,
Steven Phillips
Re: Review Request 32949: Patch for DRILL-2718
Posted by Steven Phillips <sp...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/
-----------------------------------------------------------
(Updated April 10, 2015, 8:17 p.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Summary (updated)
-----------------
Patch for DRILL-2718
Bugs: DRILL-2718
https://issues.apache.org/jira/browse/DRILL-2718
Repository: drill-git
Description (updated)
-------
DRILL-2718: Move counting and tracking of sent batches to FragmentContext
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 (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingDataTunnel.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 18b93e90853bb18971ad7a21f9a6d4bd9853c1bb
exec/java-exec/src/main/java/org/apache/drill/exec/ops/StatusHandler.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java a00df9d27a981fd6b85d3fd2a272a00c4aba80d6
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 8038527a50a1e1f6cdb6dce68b7d5438869f928f
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SendingAccountor.java 21fc800cc9855ee2da309934a4422dd37644f4bf
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java 1ef7bbd4691f0ced794e0f8d1dc3921b60044931
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java d17fdd4ba8bc3037a26cfcc791aeb18f3a112cfe
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java 6a73cdd6b4124f7410e3eb3cfaf9c7a3e17fed8d
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/Partitioner.java 9d6e98ff9ad59450bbd274ea99417a8f1acf5915
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java 33d6f9560133c4d8f97c1d214c9fa007216197ea
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java 5e21878d450825731317742638ef1e6d3f3cee30
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 8022c95821e3bac6c5d0f249cf52c85af29fd1e8
Diff: https://reviews.apache.org/r/32949/diff/
Testing
-------
No new functionality, so no new tests. Current tests all pass.
Thanks,
Steven Phillips
Re: Review Request 32949: DRILL-2718: Handle counting and status of
sent batches by FragmentContext
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/#review79695
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingDataTunnel.java
<https://reviews.apache.org/r/32949/#comment129214>
Fix the comment not to be FragmentContext-specific.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java
<https://reviews.apache.org/r/32949/#comment129215>
How about a description of what this is for?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/StatusHandler.java
<https://reviews.apache.org/r/32949/#comment129224>
This seems pretty general; would it be reusable if it weren't tied to FragmentContext? If so...
Suppose we anticipate JDK8, and define our own version of
https://docs.oracle.com/javase/8/docs/api/java/util/function/Consumer.html
as
public interface Consumer<T> {
void accept(T t);
}
and then redefine StatusHandler as
public class StatusHandler implements RpcOutcomeListener<Ack> {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(StatusHandler.class);
private final SendingAccountor sendingAccountor;
private final Consumer<RpcException> consumer;
public StatusHandler(Consumer<RpcException> consumer, SendingAccountor sendingAccountor) {
this.consumer = consumer;
this.sendingAccountor = sendingAccountor;
}
@Override
public void failed(RpcException ex) {
sendingAccountor.decrement();
consumer.accept(ex);
}
@Override
public void success(Ack value, ByteBuf buffer) {
sendingAccountor.decrement();
if (value.getOk()) {
return;
}
logger.error("Data not accepted downstream. Stopping future sends.");
// if we didn't get ack ok, we'll need to kill the query.
consumer.accept(new RpcException("Data not accepted downstream."));
}
}
Then, in FragmentContext:
private final RpcOutcomeListener<Ack> statusHandler = new StatusHandler(
new Consumer<RpcException>() {
@Override
public void accept(RpcException rpcE) {
fail(rpcE);
}
}, sendingAccountor);
- Chris Westin
On April 9, 2015, 2:44 p.m., Steven Phillips wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32949/
> -----------------------------------------------------------
>
> (Updated April 9, 2015, 2:44 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/AccountingDataTunnel.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java PRE-CREATION
> 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/ops/StatusHandler.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
>
>
Re: Review Request 32949: DRILL-2718: Handle counting and status of
sent batches by FragmentContext
Posted by Steven Phillips <sp...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32949/
-----------------------------------------------------------
(Updated April 9, 2015, 9:44 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 (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingDataTunnel.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingUserConnection.java PRE-CREATION
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/ops/StatusHandler.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
Re: Review Request 32949: DRILL-2718: Handle counting and status of
sent batches by FragmentContext
Posted by Steven Phillips <sp...@maprtech.com>.
-----------------------------------------------------------
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.
Changes
-------
A couple of small changes. Of most interest, in SendingAccountor, continue waiting after catching InterruptedException
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 (updated)
-----
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