> On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > 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.
I have some comments already but I will added a detailed explanation in PauseInjection class. > On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, > > line 183 > > <https://reviews.apache.org/r/33770/diff/1/?file=947643#file947643line183> > > > > 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. Right before the "foreman-ready" pause site, Foreman checks to make sure that if resume was called before it is completely setup that it calls resume again. The method is safe to call anytime and multiple times. If resume was called before Foreman is setup, the queryManager does not signal any fragments because there are no fragments in fragmentDataSet. > On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, > > line 184 > > <https://reviews.apache.org/r/33770/diff/1/?file=947643#file947643line184> > > > > All of them? I thought we were going to control this by queryId? Execution controls lives within FragmentContext and QueryContext of a specific query; so this resume is for all pauses within current query context. > On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, > > line 264 > > <https://reviews.apache.org/r/33770/diff/1/?file=947643#file947643line264> > > > > Should you clear the resume flag after this? No, the resume flag is to check if resume was called before Foreman is setup. It can be set to true once and remains true (since all pauses are resumed). > On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java, > > line 404 > > <https://reviews.apache.org/r/33770/diff/1/?file=947650#file947650line404> > > > > I couldn't see why you need to use a Pointer<> here instead of just the > > Exception. If an exception happens in the CancellingThread, the test does not fail. This ensures such an exception is caught. > On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java, > > line 633 > > <https://reviews.apache.org/r/33770/diff/1/?file=947650#file947650line633> > > > > Why not use assertTrue() here? If the assert fails, the exception happens in the RPC thread and the test just hangs. The check method has a workaround for that to make the test fail. > On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java, > > line 660 > > <https://reviews.apache.org/r/33770/diff/1/?file=947650#file947650line660> > > > > assertTrue()? ^ same as above. > On May 5, 2015, 9:31 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java, > > line 52 > > <https://reviews.apache.org/r/33770/diff/1/?file=947646#file947646line52> > > > > 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? I think unpause is a better name. - Sudheesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33770/#review82573 ----------------------------------------------------------- On May 2, 2015, 1:09 a.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33770/ > ----------------------------------------------------------- > > (Updated May 2, 2015, 1:09 a.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 > >
