----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33903/ -----------------------------------------------------------
(Updated May 8, 2015, 4:12 p.m.) Review request for drill, Chris Westin and Jacques Nadeau. Changes ------- added more comments around the fix and added a unit test the unit test depends on [DRILL-2757](https://issues.apache.org/jira/browse/DRILL-2757) to be committed as it uses an injection position introduced in DRILL-2757 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 (updated) ----- 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 (updated) ------- all unit tests are passing along with functional/tpch100 I will redo the tests once DRILL-2757 has been committed Thanks, abdelhakim deneche
