drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 33770: DRILL-2697 Pause injections should pause indefinitely until signalled
Date Fri, 08 May 2015 18:36:58 GMT

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


One bug, and some minor improvements.


exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
<https://reviews.apache.org/r/33770/#comment133901>

    An abstract class without any methods? What's that about?



exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
<https://reviews.apache.org/r/33770/#comment133902>

    => "a countdown latch"
    => "injector, site description, and endpoint"



exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java
<https://reviews.apache.org/r/33770/#comment133905>

    => "each of which unpauses"



exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
<https://reviews.apache.org/r/33770/#comment133906>

    Thanks for fixing this typo. It makes dealing with the code so much better for searching
and reading.



exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java
<https://reviews.apache.org/r/33770/#comment133894>

    Can we make this constructor call the other one?



exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java
<https://reviews.apache.org/r/33770/#comment133899>

    How about some javadoc for these methods?
    
    There's no need to use "public" on methods on an interface -- they're public because they're
on an interface.



exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
<https://reviews.apache.org/r/33770/#comment133896>

    You're missing an argument for the string format.



exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
<https://reviews.apache.org/r/33770/#comment133897>

    We really don't have to worry about anything here? Or should this wait (uninterruptibly)
in a loop. Please add comments explaining the rationale for doing nothing.
    
    See http://www.ibm.com/developerworks/library/j-jtp05236/



exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java
<https://reviews.apache.org/r/33770/#comment133904>

    Since this is inside the NoOpControlsInjector class, you can use a shorter name, such
as INJECTOR, or something like that, because the references will already be NoOpControlsInjector.INJECTOR.
    
    Also, is there any reason to make the instance public, since it is returned by getLatch()?
Does anyone else need to get their hands on it?



exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
<https://reviews.apache.org/r/33770/#comment133915>

    Do we really have stuff that spawns threads like this? I thought most everything submitted
Runnables to an executor service, which is slightly different. I don't think it affects anything
here, but you might mention it in a comment re how this test works.



exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
<https://reviews.apache.org/r/33770/#comment133912>

    yes, "and the test timeout mechanism will catch that case"


- Chris Westin


On May 8, 2015, 10:56 a.m., Sudheesh Katkam wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
> 
> (Updated May 8, 2015, 10:56 a.m.)
> 
> 
> Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely
for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in
the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
> 
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
> + Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns
multiple threads
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 5b28f16

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
5b4d7bd 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java
37730e3 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf

>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4

>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de

>   exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java
cf98b83 
>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8

>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java
4b1cd0c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java
80d9790 
>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java a3ceb8f 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java
b6c6852 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
c5d78cc 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 49d0c94

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 34fa639

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
ddb828c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java
0ba91b4 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java
f526fbe 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
b1c3fe0 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3

>   exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
da69e9e 
>   exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
5fa2b3f 
>   protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073 
>   protocol/src/main/protobuf/BitControl.proto 93bc33c 
>   protocol/src/main/protobuf/User.proto 59e22ae 
> 
> Diff: https://reviews.apache.org/r/33770/diff/
> 
> 
> Testing
> -------
> 
> Passes all unit tests.
> 
> 
> Thanks,
> 
> Sudheesh Katkam
> 
>


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