> On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/PhysicalPlanReader.java, > > line 91 > > <https://reviews.apache.org/r/32795/diff/1/?file=914170#file914170line91> > > > > I'm not thrilled with the pattern that's emerging here for adding > > things to the mapper. I think it would have been better to have a > > writeJson(ObjectMapper) method added to each of OptionList, > > PhysicalOperator, and ExecutionControls, and for PhysicalPlanReader just to > > have a getMapper() that is used to get the argument needed for those. In > > that form, we don't have to add a new method to PhysicalPlanReader for each > > thing that we want to add to it. We just get its mapper and write whatever > > it is to it. But it was like that before, so it might be painful to change > > it right away. If so, can you please submit a ticket to come back and > > improve this later?
Opened a JIRA [DRILL-2686](https://issues.apache.org/jira/browse/DRILL-2686) > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java, > > line 193 > > <https://reviews.apache.org/r/32795/diff/1/?file=914180#file914180line193> > > > > Do you actually need this to be protected? You set it (indirectly) via > > super(), and then you validate it in your own constructor, where you have > > the value because it is an argument. I was using defaultValue in JsonStringValidator, but I don't need it anymore. I'll revert the change. > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java, > > line 108 > > <https://reviews.apache.org/r/32795/diff/1/?file=914186#file914186line108> > > > > OK, but explain here how it is used to prevent that. Also, the class > > javadoc comment at the top should say something about the expected > > single-session non-concurrent uses of this class -- it's not thread safe, > > right? The class in thread safe. Puts are synchronized (and exceptions counts are kept atomically). > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java, > > line 74 > > <https://reviews.apache.org/r/32795/diff/1/?file=914187#file914187line74> > > > > I'd prefer that this class not know about QueryContext or > > FragmentContext. We're having to create multiple overloads to handle that. > > Can't we have these functions take an ExecutionControls object, and have > > the calls sites elsewhere pass that in by doing their own > > getExecutionControls()? I noticed this too. However, the injector gathers ExecutionControls, QueryId (and converts to string), DrillbitEndpoint differently for QueryContext and FragmentContext. As more site classes add injection points, the injector is a better place to gather those parameters. > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java, > > line 110 > > <https://reviews.apache.org/r/32795/diff/1/?file=914187#file914187line110> > > > > As above re QueryContext and FragmentContext. (But if we do have to > > keep these, at least make the private getException() take an > > ExecutionControls and have these find that from the QC or FC.) I removed the private getException() methods. > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java, > > line 70 > > <https://reviews.apache.org/r/32795/diff/1/?file=914190#file914190line70> > > > > How come you decided to go with sleeping instead of waiting for a latch > > to be triggered? This can be an enhancement as it requires a sizeable change. It is quite similar to how cancellation works. Tasks: (a) Add another message to RPC layer to signal paused remote threads to resume (through ControlHandler). Complications if the thread has not reached the pause site yet. (b) Add resume signal (like ctrl-c) to sqlline (further enhancement: another signal to trigger pause from sqlline) Anything else? > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java, > > line 245 > > <https://reviews.apache.org/r/32795/diff/1/?file=914196#file914196line245> > > > > Why this change? sys.memory executes a fragment on every drillbit. This is a better check in comparison to counting sys.drillbits (which is updated when drillbit unregisters). > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java, > > line 100 > > <https://reviews.apache.org/r/32795/diff/1/?file=914199#file914199line100> > > > > Why this change? same as above. > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java, > > line 113 > > <https://reviews.apache.org/r/32795/diff/1/?file=914199#file914199line113> > > > > Why this change? same as above. > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjection.java, > > line 102 > > <https://reviews.apache.org/r/32795/diff/1/?file=914184#file914184line102> > > > > How does this get called (@JsonCreator) if it's private? I'd just like > > to understand how that works. Tbh, I tried and it worked :) > On April 3, 2015, 10:58 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java, > > line 185 > > <https://reviews.apache.org/r/32795/diff/1/?file=914187#file914187line185> > > > > In keeping with the Exception stuff, shouldn't this be called > > injectPause()? Yes. - Sudheesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32795/#review78815 ----------------------------------------------------------- On April 2, 2015, 11:15 p.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32795/ > ----------------------------------------------------------- > > (Updated April 2, 2015, 11:15 p.m.) > > > Review request for drill and Chris Westin. > > > Repository: drill-git > > > Description > ------- > > [DRILL-2383](https://issues.apache.org/jira/browse/DRILL-2383): Inject > exceptions and pauses in various components of Drill > > + Controls can be introduced in any class that has access to FragmentContext > or QueryContext > + Controls can be fired by altering the DRILLBIT_CONTROL_INJECTIONS session > option > + Renames: SimulatedExceptions => ExecutionControls, ExceptionInjector => > ExecutionControlsInjector > + Instructions to add other types of injections are in Injection > + ExecutionControls are added as a side effect of altering > DRILLBIT_CONTROL_INJECTIONS session option > + Added injection sites in Foreman, DrillSqlWorker, FragmentExecutor > > + Unit tests in TestDrillbitResilience, TestExceptionInjection and > TestPauseInjection > > Other commits included: > > + [DRILL-2437](https://issues.apache.org/jira/browse/DRILL-2437): Moved > ExecutionControls from DrillbitContext to UserSession > + [DRILL-2382](https://issues.apache.org/jira/browse/DRILL-2382): Added > address and port to InjectionOption to specify drillbit > > Other bugs fixed: > > + BUG: Only one SystemRecord (static instance) exists per node. This causes > race conditions when multiple > drillbits run on the same node. FIX: create a new instance per request. > > Other edits: > > + Added QueryId back to QueryContext > + Log query id as string in DrillClient, WorkEventBus, QueryResultHandler > + Use try..catch block only around else clause for OptionList in > FragmentContext > + Fixed drillbitContext spelling error in QueryContext > + Show explicitly that submitWork() returns queryId in UserServer > + Added an alternative way to generate sources in protocol/readme.txt > + Added JSONStringValidator to TypeValidators > > > ===== > USAGE: > > Current checked exception sites: > > + Foreman: run-try-beginning (ForemanException), run-try-end > (ForemanException), send-fragments (ForemanException) > + DrillSqlWorker: sql-parsing (ForemanSetupException) > + FragmentExecutor: fragment-execution (IOException) > > Current pause sites: > > + Foreman: pause-run-plan > > To set controls: > ``` > > ALTER SESSION SET `drill.exec.testing.controls`='{ > "injections":[ > { > "type":"exception", > "siteClass": "org.apache.drill.exec.work.fragment.FragmentExecutor", > "desc": "fragment-execution", > "nSkip": 0, > "nFire": 1, > "exceptionClass": "java.io.IOException", > "drillbitAddress": "10.10.10.10", > "userPort": 31019 > }, > { > "type":"pause", > "siteClass": "org.apache.drill.exec.work.foreman.Foreman", > "desc": "pause-run-plan", > "millis": 5000 > } > ] }'; > ``` > NOTE: > (1) If controls are specified, they are passed to every fragment as part of > PlanFragment. Then onwards, ExecutionControls live in FragmentContext. And > since FragmentContext is created for every fragment, injections will be fired > on ALL fragments. > (2) drillbitAddress and userPort are optional. If they are set, that > injection will be fired ONLY on specified drillbit. If they are not set, the > injection will be fired on EVERY drillbit. > > > Diffs > ----- > > exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java > 14e6ad1 > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > 6d4c86c > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java > a4ac724 > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java > 3b51a69 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/PhysicalPlanReader.java > 8d77136 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java > 66ba229 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java > 710418b > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java > dc63ef9 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java > a5a5441 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java > c05b127 > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java > c76d324 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java > efb0cdf > > exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java > dbf3c74 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java > 608fac7 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java > b9721cc > > exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/MemoryRecord.java > 9cb001d > > exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/SystemTable.java > 2c338ca > > exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/ThreadsRecord.java > b184880 > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjection.java > 68cbf08 > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjector.java > 54bc351 > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/InjectionSite.java > 9e19fdd > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/SimulatedExceptions.java > 0292c08 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > 285b75a > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java > a7e6c46 > exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java e673230 > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java > 6bf23ec > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java > 9bc0552 > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ExceptionInjectionUtil.java > bf93dee > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java > d0c0279 > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java > PRE-CREATION > protocol/readme.txt bd516d3 > protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 813d961 > protocol/src/main/java/org/apache/drill/exec/proto/SchemaBitControl.java > 5e7562e > protocol/src/main/java/org/apache/drill/exec/proto/beans/PlanFragment.java > f6fbce1 > protocol/src/main/protobuf/BitControl.proto 0424725 > > Diff: https://reviews.apache.org/r/32795/diff/ > > > Testing > ------- > > Unit tests in TestDrillbitResilience, TestExceptionInjection and > TestPauseInjection. > Successful Jenkins builds. > > > Thanks, > > Sudheesh Katkam > >
