You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Jacques Nadeau <ja...@gmail.com> on 2015/06/22 02:40:18 UTC
Review Request 35719: DRILL-3242: Update RPC layer so that requests
and response are managed on a secondary thread.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35719/
-----------------------------------------------------------
Review request for drill, Chris Westin, Steven Phillips, and Sudheesh Katkam.
Repository: drill-git
Description
-------
- Add new SerializedExecutor (by cwestin) to manage serialized off-thread executions
- Create a separate serialized executor for fragment receiverFinished events.
- Update serialized executor to pool object creation.
- Ensure that FragmentExecutor acceptExternalEvents countdown occurs when only execution is cancellation.
Diffs
-----
common/src/main/java/org/apache/drill/common/SerializedExecutor.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java c642c4a
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 9ca09a1
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcConfig.java ab6c375
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlClient.java 159f1df
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 0cfa56e
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlServer.java 98ce9e1
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataClient.java 544bab9
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionCreator.java a76d753
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionManager.java 8a947a9
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandler.java 721b83e
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataRpcConfig.java c5cf498
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java b39a103
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 3f8122d
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java a197356
exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java d0a998e
exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 6fdbfca
exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 25ea307
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 5939113
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a9c2b6d
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java dc37071
Diff: https://reviews.apache.org/r/35719/diff/
Testing
-------
in progress
Thanks,
Jacques Nadeau
Re: Review Request 35719: DRILL-3242: Update RPC layer so that
requests and response are managed on a secondary thread.
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35719/#review88896
-----------------------------------------------------------
The use of the Recycler seems like an unnecessary extra complication, and it prevents making the constructor arguments final. It seems like that stuff will be so short-lived (because of the SynchronousQueues), that it'll aways be in Eden space.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java (line 433)
<https://reviews.apache.org/r/35719/#comment141482>
one blank between closing parens and opening brace
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 58)
<https://reviews.apache.org/r/35719/#comment141483>
private
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 193)
<https://reviews.apache.org/r/35719/#comment141517>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 209)
<https://reviews.apache.org/r/35719/#comment141514>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 210)
<https://reviews.apache.org/r/35719/#comment141515>
Can these be final?
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 316)
<https://reviews.apache.org/r/35719/#comment141487>
Don't you need some empty braces in the text for substitution?
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 336)
<https://reviews.apache.org/r/35719/#comment141488>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 347)
<https://reviews.apache.org/r/35719/#comment141489>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 351)
<https://reviews.apache.org/r/35719/#comment141490>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 364)
<https://reviews.apache.org/r/35719/#comment141491>
} finally {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 365)
<https://reviews.apache.org/r/35719/#comment141492>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 369)
<https://reviews.apache.org/r/35719/#comment141493>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 375)
<https://reviews.apache.org/r/35719/#comment141494>
no blank line
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 377)
<https://reviews.apache.org/r/35719/#comment141495>
no blank line
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 395)
<https://reviews.apache.org/r/35719/#comment141502>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 405)
<https://reviews.apache.org/r/35719/#comment141496>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 409)
<https://reviews.apache.org/r/35719/#comment141497>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 414)
<https://reviews.apache.org/r/35719/#comment141498>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 416)
<https://reviews.apache.org/r/35719/#comment141500>
It'd be great if these were all final.
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 417)
<https://reviews.apache.org/r/35719/#comment141499>
string description please
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 427)
<https://reviews.apache.org/r/35719/#comment141501>
} finally {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 428)
<https://reviews.apache.org/r/35719/#comment141503>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 432)
<https://reviews.apache.org/r/35719/#comment141504>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 438)
<https://reviews.apache.org/r/35719/#comment141505>
no blank line here
exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java (line 65)
<https://reviews.apache.org/r/35719/#comment141508>
We need some kind of global planning and dependency tracking.
exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java (line 118)
<https://reviews.apache.org/r/35719/#comment141509>
No blank lines here.
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java (line 492)
<https://reviews.apache.org/r/35719/#comment141510>
) {
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java (line 498)
<https://reviews.apache.org/r/35719/#comment141511>
private
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java (line 501)
<https://reviews.apache.org/r/35719/#comment141512>
unnecessary empty super()
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java (line 517)
<https://reviews.apache.org/r/35719/#comment141513>
no blank lines here
- Chris Westin
On June 21, 2015, 5:40 p.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35719/
> -----------------------------------------------------------
>
> (Updated June 21, 2015, 5:40 p.m.)
>
>
> Review request for drill, Chris Westin, Steven Phillips, and Sudheesh Katkam.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Add new SerializedExecutor (by cwestin) to manage serialized off-thread executions
> - Create a separate serialized executor for fragment receiverFinished events.
> - Update serialized executor to pool object creation.
> - Ensure that FragmentExecutor acceptExternalEvents countdown occurs when only execution is cancellation.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/SerializedExecutor.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java c642c4a
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 9ca09a1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcConfig.java ab6c375
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlClient.java 159f1df
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 0cfa56e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlServer.java 98ce9e1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataClient.java 544bab9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionCreator.java a76d753
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionManager.java 8a947a9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandler.java 721b83e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataRpcConfig.java c5cf498
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java b39a103
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 3f8122d
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java a197356
> exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java d0a998e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 6fdbfca
> exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 25ea307
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 5939113
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a9c2b6d
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java dc37071
>
> Diff: https://reviews.apache.org/r/35719/diff/
>
>
> Testing
> -------
>
> in progress
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 35719: DRILL-3242: Update RPC layer so that
requests and response are managed on a secondary thread.
Posted by Jacques Nadeau <ja...@gmail.com>.
> On July 16, 2015, 6:56 p.m., Sudheesh Katkam wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java, line 459
> > <https://reviews.apache.org/r/35719/diff/1/?file=989299#file989299line459>
> >
> > Should this just be logged? Previously, we threw the exception.
> >
> > If nothing is being done here, at least RpcBus.RpcEventHandler#runException should not just log.
We can't throw an exception since we're out of band.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35719/#review91918
-----------------------------------------------------------
On June 22, 2015, 12:40 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35719/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 12:40 a.m.)
>
>
> Review request for drill, Chris Westin, Steven Phillips, and Sudheesh Katkam.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Add new SerializedExecutor (by cwestin) to manage serialized off-thread executions
> - Create a separate serialized executor for fragment receiverFinished events.
> - Update serialized executor to pool object creation.
> - Ensure that FragmentExecutor acceptExternalEvents countdown occurs when only execution is cancellation.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/SerializedExecutor.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java c642c4a
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 9ca09a1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcConfig.java ab6c375
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlClient.java 159f1df
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 0cfa56e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlServer.java 98ce9e1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataClient.java 544bab9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionCreator.java a76d753
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionManager.java 8a947a9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandler.java 721b83e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataRpcConfig.java c5cf498
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java b39a103
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 3f8122d
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java a197356
> exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java d0a998e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 6fdbfca
> exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 25ea307
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 5939113
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a9c2b6d
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java dc37071
>
> Diff: https://reviews.apache.org/r/35719/diff/
>
>
> Testing
> -------
>
> in progress
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 35719: DRILL-3242: Update RPC layer so that
requests and response are managed on a secondary thread.
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35719/#review91918
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 426)
<https://reviews.apache.org/r/35719/#comment145636>
Should this just be logged? Previously, we threw the exception.
If nothing is being done here, at least RpcBus.RpcEventHandler#runException should not just log.
- Sudheesh Katkam
On June 22, 2015, 12:40 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35719/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 12:40 a.m.)
>
>
> Review request for drill, Chris Westin, Steven Phillips, and Sudheesh Katkam.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Add new SerializedExecutor (by cwestin) to manage serialized off-thread executions
> - Create a separate serialized executor for fragment receiverFinished events.
> - Update serialized executor to pool object creation.
> - Ensure that FragmentExecutor acceptExternalEvents countdown occurs when only execution is cancellation.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/SerializedExecutor.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java c642c4a
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 9ca09a1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcConfig.java ab6c375
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlClient.java 159f1df
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 0cfa56e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlServer.java 98ce9e1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataClient.java 544bab9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionCreator.java a76d753
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionManager.java 8a947a9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandler.java 721b83e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataRpcConfig.java c5cf498
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java b39a103
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 3f8122d
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java a197356
> exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java d0a998e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 6fdbfca
> exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 25ea307
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 5939113
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a9c2b6d
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java dc37071
>
> Diff: https://reviews.apache.org/r/35719/diff/
>
>
> Testing
> -------
>
> in progress
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 35719: DRILL-3242: Update RPC layer so that
requests and response are managed on a secondary thread.
Posted by Jacques Nadeau <ja...@gmail.com>.
> On June 30, 2015, 7:55 p.m., Sudheesh Katkam wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java, line 148
> > <https://reviews.apache.org/r/35719/diff/1/?file=989317#file989317line148>
> >
> > The RPC thread could potentially spend quite some time in this method. Why not a secondary thread here? Or is there a reason why you haven't done this already?
Not a problem since this patch moves execution off the rpc thread.
> On June 30, 2015, 7:55 p.m., Sudheesh Katkam wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java, line 207
> > <https://reviews.apache.org/r/35719/diff/1/?file=989317#file989317line207>
> >
> > Why create a SerializedExecutor? Why not simply:
> > ```java
> > fragmentContext.getExecutor().execute(new ReceiverFinished(handle));
> > ```
> > and synchronization issues could be handled by RootExec#receivingFragmentFinished (partitioned sender already does this).
It doesn't make sense to have 10s or 100s of threads to all sit on a binding variable when we can simply serialize.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35719/#review89808
-----------------------------------------------------------
On June 22, 2015, 12:40 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35719/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 12:40 a.m.)
>
>
> Review request for drill, Chris Westin, Steven Phillips, and Sudheesh Katkam.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Add new SerializedExecutor (by cwestin) to manage serialized off-thread executions
> - Create a separate serialized executor for fragment receiverFinished events.
> - Update serialized executor to pool object creation.
> - Ensure that FragmentExecutor acceptExternalEvents countdown occurs when only execution is cancellation.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/SerializedExecutor.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java c642c4a
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 9ca09a1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcConfig.java ab6c375
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlClient.java 159f1df
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 0cfa56e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlServer.java 98ce9e1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataClient.java 544bab9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionCreator.java a76d753
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionManager.java 8a947a9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandler.java 721b83e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataRpcConfig.java c5cf498
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java b39a103
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 3f8122d
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java a197356
> exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java d0a998e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 6fdbfca
> exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 25ea307
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 5939113
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a9c2b6d
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java dc37071
>
> Diff: https://reviews.apache.org/r/35719/diff/
>
>
> Testing
> -------
>
> in progress
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 35719: DRILL-3242: Update RPC layer so that
requests and response are managed on a secondary thread.
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35719/#review89808
-----------------------------------------------------------
Initial comments.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java (line 87)
<https://reviews.apache.org/r/35719/#comment142603>
unnecessary empty line?
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 60)
<https://reviews.apache.org/r/35719/#comment142640>
PONG_MESSAGE?
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 256)
<https://reviews.apache.org/r/35719/#comment142798>
final
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java (line 262)
<https://reviews.apache.org/r/35719/#comment142800>
final
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java (line 148)
<https://reviews.apache.org/r/35719/#comment142819>
The RPC thread could potentially spend quite some time in this method. Why not a secondary thread here? Or is there a reason why you haven't done this already?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java (line 206)
<https://reviews.apache.org/r/35719/#comment142602>
Why create a SerializedExecutor? Why not simply:
```java
fragmentContext.getExecutor().execute(new ReceiverFinished(handle));
```
and synchronization issues could be handled by RootExec#receivingFragmentFinished (partitioned sender already does this).
- Sudheesh Katkam
On June 22, 2015, 12:40 a.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35719/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 12:40 a.m.)
>
>
> Review request for drill, Chris Westin, Steven Phillips, and Sudheesh Katkam.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Add new SerializedExecutor (by cwestin) to manage serialized off-thread executions
> - Create a separate serialized executor for fragment receiverFinished events.
> - Update serialized executor to pool object creation.
> - Ensure that FragmentExecutor acceptExternalEvents countdown occurs when only execution is cancellation.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/SerializedExecutor.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java c642c4a
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 1cbe886
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 9ca09a1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcConfig.java ab6c375
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlClient.java 159f1df
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 0cfa56e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlServer.java 98ce9e1
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataClient.java 544bab9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionCreator.java a76d753
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionManager.java 8a947a9
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandler.java 721b83e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataRpcConfig.java c5cf498
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 80d2d6e
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java b39a103
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 3f8122d
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java a197356
> exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java d0a998e
> exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 6fdbfca
> exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 25ea307
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 5939113
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java a9c2b6d
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java dc37071
>
> Diff: https://reviews.apache.org/r/35719/diff/
>
>
> Testing
> -------
>
> in progress
>
>
> Thanks,
>
> Jacques Nadeau
>
>