You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Timothy Chen <tn...@gmail.com> on 2013/11/24 05:29:59 UTC
Review Request 15824: Patch for DRILL-281
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/
-----------------------------------------------------------
Review request for drill.
Bugs: DRILL-281
https://issues.apache.org/jira/browse/DRILL-281
Repository: drill-git
Description
-------
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247648465f20a1dd7deb7faa786412c65ab1
exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a4956e47e45a8ba5d906b55bd77f1e8e4e960
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleBroadcastExchange.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java efc0f5b8d84d6f659222f1448af14a3d8cc7e8d7
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae00296dd53becfa832e3170fc7011ea3e4
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db85538d4c8bc1aef1eed07e82874d1c28e
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSingleBroadcastExchange.java PRE-CREATION
exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
Diff: https://reviews.apache.org/r/15824/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 15824: Patch for DRILL-281
Posted by Jacques Nadeau <ja...@gmail.com>.
> On Nov. 26, 2013, 2:39 a.m., Jacques Nadeau wrote:
> > Have you done some longer testing (minute+) and made sure that you're not detecting any leaks when you run this with a large number of parallelization?
>
> Timothy Chen wrote:
> I've done some other testing manually, but didn't run any leak detection. Do we have any examples in our tests running this?
Not really. The netty leak detector recognizes when a buffer wasn't released but was gc'd. You need the test to run long enough that gc has a chance to act. Potentially putting a double System.gc() call followed by a long sleep would probably do the trick.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/#review29413
-----------------------------------------------------------
On Nov. 24, 2013, 4:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15824/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2013, 4:29 a.m.)
>
>
> Review request for drill.
>
>
> Bugs: DRILL-281
> https://issues.apache.org/jira/browse/DRILL-281
>
>
> Repository: drill-git
>
>
> Description
> -------
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247648465f20a1dd7deb7faa786412c65ab1
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a4956e47e45a8ba5d906b55bd77f1e8e4e960
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java efc0f5b8d84d6f659222f1448af14a3d8cc7e8d7
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae00296dd53becfa832e3170fc7011ea3e4
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db85538d4c8bc1aef1eed07e82874d1c28e
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15824/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 15824: Patch for DRILL-281
Posted by Timothy Chen <tn...@gmail.com>.
> On Nov. 26, 2013, 2:39 a.m., Jacques Nadeau wrote:
> > Have you done some longer testing (minute+) and made sure that you're not detecting any leaks when you run this with a large number of parallelization?
I've done some other testing manually, but didn't run any leak detection. Do we have any examples in our tests running this?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/#review29413
-----------------------------------------------------------
On Nov. 24, 2013, 4:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15824/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2013, 4:29 a.m.)
>
>
> Review request for drill.
>
>
> Bugs: DRILL-281
> https://issues.apache.org/jira/browse/DRILL-281
>
>
> Repository: drill-git
>
>
> Description
> -------
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247648465f20a1dd7deb7faa786412c65ab1
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a4956e47e45a8ba5d906b55bd77f1e8e4e960
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java efc0f5b8d84d6f659222f1448af14a3d8cc7e8d7
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae00296dd53becfa832e3170fc7011ea3e4
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db85538d4c8bc1aef1eed07e82874d1c28e
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15824/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 15824: Patch for DRILL-281
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/#review29413
-----------------------------------------------------------
Have you done some longer testing (minute+) and made sure that you're not detecting any leaks when you run this with a large number of parallelization?
- Jacques Nadeau
On Nov. 24, 2013, 4:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15824/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2013, 4:29 a.m.)
>
>
> Review request for drill.
>
>
> Bugs: DRILL-281
> https://issues.apache.org/jira/browse/DRILL-281
>
>
> Repository: drill-git
>
>
> Description
> -------
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247648465f20a1dd7deb7faa786412c65ab1
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a4956e47e45a8ba5d906b55bd77f1e8e4e960
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java efc0f5b8d84d6f659222f1448af14a3d8cc7e8d7
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae00296dd53becfa832e3170fc7011ea3e4
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db85538d4c8bc1aef1eed07e82874d1c28e
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15824/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 15824: Patch for DRILL-281
Posted by Timothy Chen <tn...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/
-----------------------------------------------------------
(Updated Dec. 8, 2013, 8:07 a.m.)
Review request for drill and Jacques Nadeau.
Bugs: DRILL-281
https://issues.apache.org/jira/browse/DRILL-281
Repository: drill-git
Description
-------
Verified refCount is 0 after broadcast, use async SendBatch call while broadcasting.
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247
exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a495
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastExchange.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/bit/BitTunnel.java 4ce826c
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestBroadcastExchange.java PRE-CREATION
exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
exec/java-exec/src/test/resources/sender/broadcast_exchange_long_run.json PRE-CREATION
Diff: https://reviews.apache.org/r/15824/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 15824: Patch for DRILL-281
Posted by Timothy Chen <tn...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/
-----------------------------------------------------------
(Updated Dec. 8, 2013, 8:06 a.m.)
Review request for drill.
Changes
-------
Verified refCount is 0 after broadcast, use async SendBatch call while broadcasting.
Bugs: DRILL-281
https://issues.apache.org/jira/browse/DRILL-281
Repository: drill-git
Description (updated)
-------
Verified refCount is 0 after broadcast, use async SendBatch call while broadcasting.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247
exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a495
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastExchange.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/bit/BitTunnel.java 4ce826c
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestBroadcastExchange.java PRE-CREATION
exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
exec/java-exec/src/test/resources/sender/broadcast_exchange_long_run.json PRE-CREATION
Diff: https://reviews.apache.org/r/15824/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 15824: Patch for DRILL-281
Posted by Timothy Chen <tn...@gmail.com>.
> On Nov. 26, 2013, 8:31 p.m., Mehant Baid wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java, line 84
> > <https://reviews.apache.org/r/15824/diff/1/?file=390554#file390554line84>
> >
> > Why are you invoking retain() on your buffers? While getting the buffers from the value vector (BaseDataValueVector) calls retain() on the buffer before releasing it so the reference count should be set for the buffers. What am I missing?
I'm calling extra retain since I'm broadcasting to multiple endpoints, so the buffers is reused in my case instead of the usual getBuffers and send.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/#review29458
-----------------------------------------------------------
On Nov. 24, 2013, 4:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15824/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2013, 4:29 a.m.)
>
>
> Review request for drill.
>
>
> Bugs: DRILL-281
> https://issues.apache.org/jira/browse/DRILL-281
>
>
> Repository: drill-git
>
>
> Description
> -------
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247648465f20a1dd7deb7faa786412c65ab1
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a4956e47e45a8ba5d906b55bd77f1e8e4e960
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java efc0f5b8d84d6f659222f1448af14a3d8cc7e8d7
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae00296dd53becfa832e3170fc7011ea3e4
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db85538d4c8bc1aef1eed07e82874d1c28e
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15824/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 15824: Patch for DRILL-281
Posted by Mehant Baid <ba...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15824/#review29458
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java
<https://reviews.apache.org/r/15824/#comment56668>
Why are you invoking retain() on your buffers? While getting the buffers from the value vector (BaseDataValueVector) calls retain() on the buffer before releasing it so the reference count should be set for the buffers. What am I missing?
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java
<https://reviews.apache.org/r/15824/#comment56669>
Could you please add comments to all the newly added classes giving a short description of their purpose.
- Mehant Baid
On Nov. 24, 2013, 4:29 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15824/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2013, 4:29 a.m.)
>
>
> Review request for drill.
>
>
> Bugs: DRILL-281
> https://issues.apache.org/jira/browse/DRILL-281
>
>
> Repository: drill-git
>
>
> Description
> -------
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java b8a7247648465f20a1dd7deb7faa786412c65ab1
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java 040a4956e47e45a8ba5d906b55bd77f1e8e4e960
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastSender.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java efc0f5b8d84d6f659222f1448af14a3d8cc7e8d7
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java e1fb3ae00296dd53becfa832e3170fc7011ea3e4
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderCreator.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java e9b56db85538d4c8bc1aef1eed07e82874d1c28e
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSingleBroadcastExchange.java PRE-CREATION
> exec/java-exec/src/test/resources/sender/broadcast_exchange.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15824/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>