> On May 8, 2015, 6:36 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java, > > line 62 > > <https://reviews.apache.org/r/33770/diff/1-2/?file=947638#file947638line62> > > > > An abstract class without any methods? What's that about?
So instances are not created. It's an annotated class that is used only by Jackson's ObjectMapper to allow a list of injections to hold various types of injections (pause, latch, exception). > On May 8, 2015, 6:36 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java, > > line 40 > > <https://reviews.apache.org/r/33770/diff/2/?file=953183#file953183line40> > > > > Can we make this constructor call the other one? Not as part of this patch, how the allcator is assigned needs to be changed. > On May 8, 2015, 6:36 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java, > > line 61 > > <https://reviews.apache.org/r/33770/diff/2/?file=953191#file953191line61> > > > > 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/ Depends on the use case. I'll add both methods, and also add InterruptedException to the signature for await(). > On May 8, 2015, 6:36 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java, > > line 51 > > <https://reviews.apache.org/r/33770/diff/2/?file=953195#file953195line51> > > > > 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? Yes, in ExecutionControls#lookupCountDownLatchInjection. When there is no injection found, we return a latch that does nothing so that code does not throw NPE. > On May 8, 2015, 6:36 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java, > > line 109 > > <https://reviews.apache.org/r/33770/diff/2/?file=953208#file953208line109> > > > > 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. Yes, in PartitionSender. - Sudheesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33770/#review83023 ----------------------------------------------------------- On May 8, 2015, 5:56 p.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33770/ > ----------------------------------------------------------- > > (Updated May 8, 2015, 5:56 p.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 > >
