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