drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sudheesh Katkam" <skat...@maprtech.com>
Subject Re: Review Request 32795: DRILL-2383: Add exception and pause injections for testing drillbit stability
Date Tue, 07 Apr 2015 16:55:38 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message