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, 14 Apr 2015 16:59:20 GMT


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java, line 77
> > <https://reviews.apache.org/r/32795/diff/3-4/?file=918266#file918266line77>
> >
> >     Why do FragmentContext and QueryContext both need their own set of ExecutionControls?

It is the same set of ExecutionControls, just in different parts of the query flow. 

If controls are specified, the set of ExecutionControls lives as a short lived option in session
options. When QueryContext is created, this option is deserialized and classes that are involved
in planning (like Foreman, SqlWorker, Optimizer) can inject controls. When FragmentContext
is created, the same option (passed in as part of PlanFragment iff injections should happen
on the query) is deserialized and classes that are involved in execution can inject controls.


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java,
line 378
> > <https://reviews.apache.org/r/32795/diff/3/?file=918268#file918268line378>
> >
> >     newBuilder() goes on the previous line, since it's not actually setting an attribute
in the builder.
> >     
> >     Do we really need these empty end-of-line comments?

You are looking at an incorrect revision. I have reverted changes to this file in my latest
patch.


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java,
line 391
> > <https://reviews.apache.org/r/32795/diff/3/?file=918268#file918268line391>
> >
> >     If you are going to use these end-of-line comments, then always space them the
same way, and do it on all lines

I have reverted changes to this file in my latest patch.


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java,
line 116
> > <https://reviews.apache.org/r/32795/diff/3-4/?file=918269#file918269line116>
> >
> >     This doesn't look like an exact substitution; what's this change about?

I did not make this change. It was brought in when I rebased. Please look at [4](https://reviews.apache.org/r/32795/diff/4/)
and not [3-4](https://reviews.apache.org/r/32795/diff/3-4/)


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java,
line 33
> > <https://reviews.apache.org/r/32795/diff/4/?file=926772#file926772line33>
> >
> >     Can we get a comment describing what this is?

Okay, I will add comments.


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java,
line 40
> > <https://reviews.apache.org/r/32795/diff/4/?file=926772#file926772line40>
> >
> >     Comments for the arguments?

Okay, I will add comments.


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java,
line 42
> > <https://reviews.apache.org/r/32795/diff/4/?file=926772#file926772line42>
> >
> >     Should this be validated? Is any value ok?

Need to validate.


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java,
line 68
> > <https://reviews.apache.org/r/32795/diff/4/?file=926772#file926772line68>
> >
> >     -1 has a special meaning?

Yes, that the query is not short lived.


> On April 14, 2015, 4:28 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java,
line 88
> > <https://reviews.apache.org/r/32795/diff/4/?file=926773#file926773line88>
> >
> >     This looks stateless -- could it be static?

No, it accesses shortLivedOptions which is an instance member.


- Sudheesh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32795/#review80037
-----------------------------------------------------------


On April 14, 2015, 4:45 a.m., Sudheesh Katkam wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32795/
> -----------------------------------------------------------
> 
> (Updated April 14, 2015, 4:45 a.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): Support to inject exceptions
and pauses in various components of Drill
> 
> + Controls can be introduced in any class that has access to FragmentContext/QueryContext
> + Controls are fired only if assertions are enabled
> + Controls can be fired by altering the DRILLBIT_CONTROL_INJECTIONS session option
> + Renames: SimulatedExceptions => ExecutionControls, ExceptionInjector => ExecutionControlsInjector
> + 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 FragmentContext/QueryContext
> + [DRILL-2382](https://issues.apache.org/jira/browse/DRILL-2382): Added address and port
to Injection to specify drillbit
> + [DRILL-2384](https://issues.apache.org/jira/browse/DRILL-2384): Added QueryState to
SingleRowListener and assert that state is COMPLETED while testing
> 
> Other edits:
> 
> + Support for short lived session options in SessionOptionManager (using TTL in OptionValidator)
> + Introduced query count in UserSession
> + Added QueryState to queryCompleted() in UserResultsListener to check if COMPLETED/CANCELED
> + Added JSONStringValidator to TypeValidators
> + 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
> + Do not call setLocalOption twice in FallbackOptionManager
> + Show explicitly that submitWork() returns queryId in UserServer
> + Updated protocol/readme.txt to include an alternative way to generate sources
> 
> 
> =====
> 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",
>         "address": "10.10.10.10",
>         "port": 31019
>     },
>     {
>         "type":"pause",
>         "siteClass": "org.apache.drill.exec.work.foreman.Foreman",
>         "desc": "pause-run-plan",
>         "nSkip": 0,
>         "nFire": 1,
>         "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) address and port 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 579cf7d

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

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java 2fa0b18 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
b98778d 
>   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/FallbackOptionManager.java
45d393c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java
4ffe9a3 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java
43071e7 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java
c3de190 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
1a8559e 
>   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/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/InjectionConfigurationException.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/NoOpControlsInjector.java
PRE-CREATION 
>   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/main/java/org/apache/drill/exec/work/user/UserWorker.java 854f474

>   exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 0c2f0e5 
>   exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java 264123f 
>   exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java 5703bf9 
>   exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java
f5f5b8d 
>   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/impl/DrillResultSetImpl.java 24ef62b

>   protocol/readme.txt bd516d3 
> 
> 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