drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdelhakim deneche" <adene...@gmail.com>
Subject Re: Review Request 33903: DRILL-2878: FragmentExecutor.closeOutResources() is not called if an exception happens in the Foreman before the fragment executor starts running
Date Sat, 09 May 2015 18:07:01 GMT


> On May 8, 2015, 4:35 p.m., Chris Westin wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/TestFragmentExecutorCancel.java,
line 56
> > <https://reviews.apache.org/r/33903/diff/2/?file=953149#file953149line56>
> >
> >     To make sure you get the expected exception here, declare a boolean variable
initialized to false before the try block. Then set it to true inside the catch block. After
the catch block use assertTrue() on it.

I added an Assert.fail() after the query call, it will fail the unit test if the query runs
successfully.


- abdelhakim


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


On May 9, 2015, 6:06 p.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33903/
> -----------------------------------------------------------
> 
> (Updated May 9, 2015, 6:06 p.m.)
> 
> 
> Review request for drill, Chris Westin and Jacques Nadeau.
> 
> 
> Bugs: DRILL-2878
>     https://issues.apache.org/jira/browse/DRILL-2878
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> *INITIAL-PATCH*
> 
> This is a "quick fix" that seem to solve the problem, at least for the cases I am able
to reproduce it for. closeOutResources() shouldn't throw any exception at this point because
we didn't even start running, and any allocation failures will be suppressed (do we want this
?)
> 
> If this fix is acceptable I will go ahead and add a "private volatile boolean startedRunning"
that will be set to true in run() and used in cancel() to check if we need to call closeOutResources().
> 
> I will also add a unit test, I know how to reproduce the problem for both the root and
intermediate fragments, but I still need to find a proper way to detect that those fragments
were not closed properly.
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
ddb828c 
>   exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/TestFragmentExecutorCancel.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/33903/diff/
> 
> 
> Testing
> -------
> 
> all unit tests are passing along with functional/tpch100
> 
> I will redo the tests once DRILL-2757 has been committed
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


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