----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34173/#review83655 -----------------------------------------------------------
A couple minor things, and a suggestion on how to get even more out of this test by testing the same queries with failures at multiple different locations. exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java <https://reviews.apache.org/r/34173/#comment134664> There could be more than one injection site, so this should have a more specific name. exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java <https://reviews.apache.org/r/34173/#comment134665> It took me a minute to understand this; it is an unusual way to use the exception injector, so please add a comment explaining what's going on here so others that aren't as familiar with it or the allocator (they might have assumed this should be throwing OutOfMemoryException) will understand why you catch the exception and return null. exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java <https://reviews.apache.org/r/34173/#comment134666> "Exception{" => "Exception {" throughout the file. exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java <https://reviews.apache.org/r/34173/#comment134667> Could this method be put into BaseTestQuery so that other tests could use it? Maybe it should be TestWithOutOfMemoryException then. exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java <https://reviews.apache.org/r/34173/#comment134668> I might parameterize the nSkip value, so that these queries could be tested with this exception being thrown in different places. For example, we might define a fixed set of numbers like final int nSkips[] = {200, 250, 300, 375}; testWithException takes a single number as an additional argument. testWithExeptions might take an array, and call testWithException in a loop with each of the values. Then we could test each query with failures at different locations. As above, these might be useful in BaseTestQuery, but if that's too hard for now, just do it here. - Chris Westin On May 13, 2015, 10:54 a.m., abdelhakim deneche wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34173/ > ----------------------------------------------------------- > > (Updated May 13, 2015, 10:54 a.m.) > > > Review request for drill, Chris Westin and Jacques Nadeau. > > > Bugs: DRILL-3053 > https://issues.apache.org/jira/browse/DRILL-3053 > > > Repository: drill-git > > > Description > ------- > > added an unchecked injection site in ChildAllocator.buffer(int, int). It's > only enabled when assertions are enabled. > You can throw any unchecked or Error exception, but if you throw a > NullPointerException the buffer() method will just return "null" without > throwing any exception to simulate a "normal" allocation failure > > > Diffs > ----- > > > common/src/main/java/org/apache/drill/common/exceptions/DrillRuntimeException.java > abc7065 > common/src/main/java/org/apache/drill/common/exceptions/UserException.java > a67cb3f > > exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java > 9670c7e > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java > 387d300 > exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/34173/diff/ > > > Testing > ------- > > pending results from unit tests / cluster > > > Thanks, > > abdelhakim deneche > >
