You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by abdelhakim deneche <ad...@gmail.com> on 2015/05/06 20:46:43 UTC
Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources() is
not called if an exception happens in the Foreman before the fragment
executor starts running
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33903/
-----------------------------------------------------------
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
Diff: https://reviews.apache.org/r/33903/diff/
Testing
-------
Thanks,
abdelhakim deneche
Re: Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources()
is not called if an exception happens in the Foreman before the fragment
executor starts running
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33903/#review82729
-----------------------------------------------------------
Ship it (non-binding)
Why do you need startedRunning if a null root is meant to detect the same thing?
- Chris Westin
On May 6, 2015, 11:46 a.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33903/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 11:46 a.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
>
> Diff: https://reviews.apache.org/r/33903/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> abdelhakim deneche
>
>
Re: Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources()
is not called if an exception happens in the Foreman before the fragment
executor starts running
Posted by abdelhakim deneche <ad...@gmail.com>.
> 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
>
>
Re: Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources()
is not called if an exception happens in the Foreman before the fragment
executor starts running
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33903/#review83005
-----------------------------------------------------------
One improvement for the unit test, but otherwise LGTM (non-binding).
exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/TestFragmentExecutorCancel.java
<https://reviews.apache.org/r/33903/#comment133883>
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.
- Chris Westin
On May 8, 2015, 9:12 a.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33903/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 9:12 a.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
>
>
Re: Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources()
is not called if an exception happens in the Foreman before the fragment
executor starts running
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33903/
-----------------------------------------------------------
(Updated May 10, 2015, 3:39 p.m.)
Review request for drill, Chris Westin and Jacques Nadeau.
Changes
-------
fixed WorkerBee.startFragmentPendingRemote() to ignore cancelled fragments
small fix in unit test
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/WorkManager.java a3ceb8f
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
Re: Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources()
is not called if an exception happens in the Foreman before the fragment
executor starts running
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33903/#review83173
-----------------------------------------------------------
Just a minor cosmetic tweak, then ship it (non-binding).
exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/TestFragmentExecutorCancel.java
<https://reviews.apache.org/r/33903/#comment134055>
That works too.
It's more traditional to do a static import of Assert.fail() (just like the other assertXXX() functions), and then just use fail(...).
- Chris Westin
On May 9, 2015, 11:06 a.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33903/
> -----------------------------------------------------------
>
> (Updated May 9, 2015, 11:06 a.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
>
>
Re: Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources()
is not called if an exception happens in the Foreman before the fragment
executor starts running
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
unit test fails if the query didn't fail expected
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
-------
all unit tests are passing along with functional/tpch100
I will redo the tests once DRILL-2757 has been committed
Thanks,
abdelhakim deneche
Re: Review Request 33903: DRILL-2878:
FragmentExecutor.closeOutResources()
is not called if an exception happens in the Foreman before the fragment
executor starts running
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
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