----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33770/#review82573 -----------------------------------------------------------
A few minor things, along with some questions about resumeAll and the implications of that. I may have more suggestions based on the answers to my questions. Some of those might be best answered in the form of comments in the code or class javadoc because it seems likely others will have the same questions later. exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java <https://reviews.apache.org/r/33770/#comment133305> I'm not sure about this waiting uninterruptibly. We're going to start using Thread.interrupt() to regain control of threads that are blocked waiting on queues or sockets if the fragment they are running on behalf of is cancelled. Seems like this should be handled in the same way. I can talk to you about that more, and Venki can tell you about what he's doing in this area. exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/33770/#comment133310> Should this wait on acceptExternalEvents, in the same way cancel() does? What happens if we're not completely set up and we execute these? The queryContext one seems like it might be ok, but the queryManager one definitely seems like it could be a problem if this comes in before all the remote fragments are set up. exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/33770/#comment133307> All of them? I thought we were going to control this by queryId? exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java <https://reviews.apache.org/r/33770/#comment133309> Should you clear the resume flag after this? exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java <https://reviews.apache.org/r/33770/#comment133316> Does this mean that I can't have multiple pauses in the execution thread that will all work for a single query? For example, suppose I inject two pauses at different phases of execution: one just after planning and remote fragments are set up, and another after the first batch of results are returned. Will they both work, or will only the first one work? exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java <https://reviews.apache.org/r/33770/#comment133319> How about some javadoc explaining what this means? From this it's not at all obvious that this is about injected pauses instead of something like suspend/resume. Would "unpause()" be a better name? exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/33770/#comment133322> I couldn't see why you need to use a Pointer<> here instead of just the Exception. exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/33770/#comment133323> Instead of "TC 1", "TC 2", etc, can you please just copy-paste the one-line descriptions of these test cases from the original. Much easier if it's all described here inline. exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/33770/#comment133324> Why not use assertTrue() here? exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/33770/#comment133325> assertTrue()? exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java <https://reviews.apache.org/r/33770/#comment133327> break the line at .setUserName() so only one attribute is set per line. protocol/src/main/protobuf/BitControl.proto <https://reviews.apache.org/r/33770/#comment133329> Can we clean up this whitespace, and leave just a newline? - Chris Westin On May 1, 2015, 6:09 p.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33770/ > ----------------------------------------------------------- > > (Updated May 1, 2015, 6:09 p.m.) > > > Review request for drill, Chris Westin and Jacques Nadeau. > > > Repository: drill-git > > > Description > ------- > > 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. > > + Fix for bug in FragmentExecutor#closeOutResources > + Better error messages and more tests in TestDrillbitResilience and > TestPauseInjection > + Added execution controls to operator context > + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to > ControlMessageHandler > > > Diffs > ----- > > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > ae0f580 > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java > ccafa67 > > 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 > b7ffbf0 > > 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/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 > edbcfde > > 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 > 0783fee > > 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 > 2698701 > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java > 508b10c > protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 813d961 > 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 0424725 > protocol/src/main/protobuf/User.proto 59e22ae > > Diff: https://reviews.apache.org/r/33770/diff/ > > > Testing > ------- > > Passes all unit tests. > > > Thanks, > > Sudheesh Katkam > >
