----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33770/#review83023 -----------------------------------------------------------
One bug, and some minor improvements. exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java <https://reviews.apache.org/r/33770/#comment133901> An abstract class without any methods? What's that about? exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java <https://reviews.apache.org/r/33770/#comment133902> => "a countdown latch" => "injector, site description, and endpoint" exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java <https://reviews.apache.org/r/33770/#comment133905> => "each of which unpauses" exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java <https://reviews.apache.org/r/33770/#comment133906> Thanks for fixing this typo. It makes dealing with the code so much better for searching and reading. exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java <https://reviews.apache.org/r/33770/#comment133894> Can we make this constructor call the other one? exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java <https://reviews.apache.org/r/33770/#comment133899> How about some javadoc for these methods? There's no need to use "public" on methods on an interface -- they're public because they're on an interface. exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java <https://reviews.apache.org/r/33770/#comment133896> You're missing an argument for the string format. exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java <https://reviews.apache.org/r/33770/#comment133897> We really don't have to worry about anything here? Or should this wait (uninterruptibly) in a loop. Please add comments explaining the rationale for doing nothing. See http://www.ibm.com/developerworks/library/j-jtp05236/ exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java <https://reviews.apache.org/r/33770/#comment133904> Since this is inside the NoOpControlsInjector class, you can use a shorter name, such as INJECTOR, or something like that, because the references will already be NoOpControlsInjector.INJECTOR. Also, is there any reason to make the instance public, since it is returned by getLatch()? Does anyone else need to get their hands on it? exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java <https://reviews.apache.org/r/33770/#comment133915> Do we really have stuff that spawns threads like this? I thought most everything submitted Runnables to an executor service, which is slightly different. I don't think it affects anything here, but you might mention it in a comment re how this test works. exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java <https://reviews.apache.org/r/33770/#comment133912> yes, "and the test timeout mechanism will catch that case" - Chris Westin On May 8, 2015, 10:56 a.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33770/ > ----------------------------------------------------------- > > (Updated May 8, 2015, 10:56 a.m.) > > > Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti. > > > Repository: drill-git > > > Description > ------- > > [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites > wait indefinitely for a resume signal > DrillClient sends a resume signal to UserServer. UserServer triggers a resume > call in the correct Foreman. Foreman resumes all pauses related to the query > through the Control layer. > > + Better error messages and more tests in TestDrillbitResilience and > TestPauseInjection > + Added execution controls to operator context > + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to > ControlMessageHandler > + Added CountDownLatchInjection, useful in cases like ParititionedSender that > spawns multiple threads > > > Diffs > ----- > > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > 5b28f16 > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java > 7cc52ba > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java > 6dbd880 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java > 5b4d7bd > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java > 37730e3 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java > a4f9fdf > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java > 88592d4 > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java > 9e929de > > exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java > cf98b83 > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java > 1171bf8 > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java > 4b1cd0c > exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java > 96fed3a > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java > 80d9790 > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java > e5f9c9c > exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java > a3ceb8f > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java > b6c6852 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java > c5d78cc > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > 49d0c94 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java > 34fa639 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java > ddb828c > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java > 0ba91b4 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java > f526fbe > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java > b1c3fe0 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java > 8854ef3 > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java > da69e9e > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java > 5fa2b3f > protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976 > protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47 > protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java > 4d03073 > protocol/src/main/protobuf/BitControl.proto 93bc33c > protocol/src/main/protobuf/User.proto 59e22ae > > Diff: https://reviews.apache.org/r/33770/diff/ > > > Testing > ------- > > Passes all unit tests. > > > Thanks, > > Sudheesh Katkam > >
