----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32795/#review78815 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/planner/PhysicalPlanReader.java <https://reviews.apache.org/r/32795/#comment127847> 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? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/32795/#comment127848> Here's where my comments about the JSON writing matter. In the formulation I suggest, we'd instead have final ObjectMapper mapper = reader.getMapper(); options.writeJson(mapper); executionControls.writeJson(mapper); So as we add more things to the plan, we don't have to add more methods to it. Each object knows how to write itself, given the mapper. And if we ever need to add them to anything else, that object just needs to expose its mapper in a similar way, rather than having a method per item. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/32795/#comment127849> Thank you for cleaning up these bogus trailing comments -- I'm grateful to see you help clean up the code. In the first one, .newBuilder() should go on the same line as the class name, like it does below for PlanFragment. exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java <https://reviews.apache.org/r/32795/#comment127850> Woohoo, very cool! exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java <https://reviews.apache.org/r/32795/#comment127851> Minor nit, but I'd like to start getting folks to mark these as // TODO(DRILL-2529): ...... when there's an associated ticket. exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java <https://reviews.apache.org/r/32795/#comment127890> Does the default toString() for classes give the class name? I would have used getClassName() just to be sure of the result here. exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java <https://reviews.apache.org/r/32795/#comment127891> 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. exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjection.java <https://reviews.apache.org/r/32795/#comment127893> How does this get called (@JsonCreator) if it's private? I'd just like to understand how that works. exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java <https://reviews.apache.org/r/32795/#comment127895> 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? exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java <https://reviews.apache.org/r/32795/#comment127896> Ok, but please tell us why it should only be called from there. exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java <https://reviews.apache.org/r/32795/#comment127898> 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()? exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java <https://reviews.apache.org/r/32795/#comment127899> 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.) exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java <https://reviews.apache.org/r/32795/#comment127900> In keeping with the Exception stuff, shouldn't this be called injectPause()? exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java <https://reviews.apache.org/r/32795/#comment127901> Same comment as above re QueryContext and FragmentContext; I'd rather not have this class know about them. exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java <https://reviews.apache.org/r/32795/#comment127913> How come you decided to go with sleeping instead of waiting for a latch to be triggered? exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/32795/#comment127916> Why this change? exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/32795/#comment127919> How do you know the pause actually did anything? The query may come back as canceled, but you don't know that the pause took effect. You should take note of the time before you start, and after completion, subtract, and assert that this is >= pauseOption.millis . exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java <https://reviews.apache.org/r/32795/#comment127921> Why this change? exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java <https://reviews.apache.org/r/32795/#comment127922> Why this change? exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java <https://reviews.apache.org/r/32795/#comment127924> Because of CPU scheduling and such, I'd just check that time >= . And since you've checked the pause here, you can ignore my earlier request for that in the other test. - Chris Westin On April 2, 2015, 4: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, 4: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 > >
