> On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java, > > line 44 > > <https://reviews.apache.org/r/32795/diff/3/?file=918273#file918273line44> > > > > Why is this change part of this patch?
Currently, the QueryState is not passed back to the Client. This is used in TestDrillbitResilience to check if the queries have completed (COMPLETED or CANCELED). > On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java, > > line 101 > > <https://reviews.apache.org/r/32795/diff/3/?file=918274#file918274line101> > > > > why the change? I made the change to explicitly call out that submitWork() returns a QueryId, which is sent as a response (useful while debugging, and understanding query flow). > On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java, > > line 111 > > <https://reviews.apache.org/r/32795/diff/3/?file=918274#file918274line111> > > > > why the change? ^ same thing as above. I found it useful to call out the return type of cancelQuery(). > On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/MemoryRecord.java, > > line 45 > > <https://reviews.apache.org/r/32795/diff/3/?file=918280#file918280line45> > > > > why is this called dummy instance? This instance is used only to find the field names and field sql type names, and not actually assign values. > On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/MemoryRecord.java, > > line 86 > > <https://reviews.apache.org/r/32795/diff/3/?file=918280#file918280line86> > > > > why remove? people shouldn't be creating a memory record, right? Correct. However, when multiple drillbits run in the same JVM, e.g. in test classes, this causes race conditions as multiple threads are trying to access the same static instance e.g. I have seen NPEs because one thread was allocating ByteBufs and another was assigning values to ByteBufs. > On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/ThreadsRecord.java, > > line 74 > > <https://reviews.apache.org/r/32795/diff/3/?file=918282#file918282line74> > > > > same, why remove? ^ same as above. > On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjection.java, > > line 210 > > <https://reviews.apache.org/r/32795/diff/3/?file=918283#file918283line210> > > > > Why do we need a custom serializer here? The "exceptionClass" field is serialized as, for example, "class ForemanException" and not "ForemanException" because the default call is to the toString() method. But I just added a getter with a @JsonProperty("exceptionClass"). > On April 7, 2015, 2:19 a.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExceptionInjection.java, > > line 47 > > <https://reviews.apache.org/r/32795/diff/3/?file=918283#file918283line47> > > > > Can you please move to using JsonCreator and doing all validation at > > deserialization time? This should also remove the need for two separate > > classes. Incorrect values should throw a ValidationException. Validation at deserialization time would not allow that, which is why there are two classes. - Sudheesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32795/#review79108 ----------------------------------------------------------- On April 6, 2015, 10:53 p.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32795/ > ----------------------------------------------------------- > > (Updated April 6, 2015, 10:53 p.m.) > > > Review request for drill, abdelhakim deneche, Chris Westin, and Jacques > Nadeau. > > > 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 > bd93206 > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > 9a948fb > > exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java > 98948af > 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 > d6f25fb > > 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 > a1be83b > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java > 934a094 > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java > 877bc08 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java > 19d77b0 > > 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/server/rest/QueryWrapper.java > fbbf0b8 > > 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 > 23ef0d3 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java > a7e6c46 > exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 725594a > exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java e673230 > exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java > 5703bf9 > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java > ba905c4 > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java > e03098a > > exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java > 55f0d75 > > exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetPhysicalPlan.java > 882cdbd > > 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 > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java fb27d2d > 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 > >
