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