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 34690: DRILL-2903 Update TestDrillbitResilience tests so that they correctly manage canceled queries that get to complete too quickly
Date Thu, 28 May 2015 18:59:12 GMT


> On May 28, 2015, 4:22 a.m., abdelhakim deneche wrote:
> > is the description found in JIRA 2903 relevant to this patch ?

DRILL-2967 is sufficient to fix what the JIRA requires. I am adding additional code like repititions
to make the tests more robust.


> On May 28, 2015, 4:22 a.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java,
line 358
> > <https://reviews.apache.org/r/34690/diff/1/?file=972384#file972384line358>
> >
> >     I know we should try to cleanup the code with each new patch we submit, but
changing files just to clean them makes it a little harder to review the "real" changes involved
in this patch.

This change is not "just to clean". Not having this annotation can have unforeseen consequences
e.g. if we remove the close() method from the interface, the compiler complains if the annotation
is present. Additionally, any calls to the method within the class would also need to be removed.
There was bug introduced in PojoRecordReader because of something like this. Instead of filing
a JIRA, I made this change part of my patch.


> On May 28, 2015, 4:22 a.m., abdelhakim deneche wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java,
line 110
> > <https://reviews.apache.org/r/34690/diff/1/?file=972392#file972392line110>
> >
> >     how doew junit handle timeouts when we repeat a test ? do we get timeouts if
we increase the repeat count too much ??

The timeout is per iteration.


> On May 28, 2015, 4:22 a.m., abdelhakim deneche wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java,
line 189
> > <https://reviews.apache.org/r/34690/diff/1/?file=972392#file972392line189>
> >
> >     ZookeeperHelper javadoc already explains what happens when we pass "true" to
it's constructor. We don't need to repeat the explanation here.

I think we should repeat it. startSomeDrillbits() has a series of comments to explain how
the cluster is being configured, and additional comments never hurt.


- Sudheesh


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


On May 27, 2015, 12:52 a.m., Sudheesh Katkam wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34690/
> -----------------------------------------------------------
> 
> (Updated May 27, 2015, 12:52 a.m.)
> 
> 
> Review request for drill, abdelhakim deneche, Chris Westin, and Venki Korukanti.
> 
> 
> Bugs: DRILL-2903
>     https://issues.apache.org/jira/browse/DRILL-2903
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> DRILL-2903: General improvements in TestDrillbitResilience
> + Added RepeatTestRule to repeat tests
> + Added ControlsInjectionBuilder to build controls in tests
> + Added @Ignore and filed JIRAs for failing tests
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/drill/common/util/RepeatTestRule.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/util/TestTools.java 5be8d40 
>   common/src/test/java/org/apache/drill/test/DrillTest.java bbe014f 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java 6176f77

>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/ProtobufLengthDecoder.java 4f075d3

>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java 16b9b63

>   exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
561d816 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
8ee7d38 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 5d07b49

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 71b77c6

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
e5e0700 
>   exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
696aed8 
>   exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionBuilder.java
PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java
346c6dd 
>   exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
c98f54c 
>   exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java
e3558a1 
>   exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
ba29c58 
> 
> Diff: https://reviews.apache.org/r/34690/diff/
> 
> 
> Testing
> -------
> 
> Currently running unit tests. Need to run regression tests.
> 
> 
> Thanks,
> 
> Sudheesh Katkam
> 
>


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