You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Sudheesh Katkam <sk...@maprtech.com> on 2015/05/29 22:44:37 UTC
Review Request 34829: DRILL-3190: Invalid FragmentState transition
from CANCELLATION_REQUESTED in QueryManager
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/
-----------------------------------------------------------
Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
Bugs: DRILL-3190
https://issues.apache.org/jira/browse/DRILL-3190
Repository: drill-git
Description
-------
DRILL-3190: Check for transition from CANCELLATION_REQUESTED to non-terminal state in FragmentData
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
Diff: https://reviews.apache.org/r/34829/diff/
Testing
-------
Running unit and regresssion tests.
Thanks,
Sudheesh Katkam
Re: Review Request 34829: DRILL-3190: Invalid FragmentState
transition from CANCELLATION_REQUESTED in QueryManager
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 29, 2015, 9:49 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java, line 69
> > <https://reviews.apache.org/r/34829/diff/1/?file=974693#file974693line69>
> >
> > Foreman state management is already split between the Foreman and the QueryManager. This will make the FragmentData handle part of that logic too!
> >
> > Shouldn't we move this to the QueryManager instead ? at least we would have one less class to check involved
Will make the change :)
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review85822
-----------------------------------------------------------
On May 29, 2015, 8:44 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34829/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 8:44 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
>
>
> Bugs: DRILL-3190
> https://issues.apache.org/jira/browse/DRILL-3190
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3190: Check for transition from CANCELLATION_REQUESTED to non-terminal state in FragmentData
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
>
> Diff: https://reviews.apache.org/r/34829/diff/
>
>
> Testing
> -------
>
> Running unit and regresssion tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34829: DRILL-3190: Invalid FragmentState
transition from CANCELLATION_REQUESTED in QueryManager
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review85822
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/34829/#comment137643>
Foreman state management is already split between the Foreman and the QueryManager. This will make the FragmentData handle part of that logic too!
Shouldn't we move this to the QueryManager instead ? at least we would have one less class to check involved
- abdelhakim deneche
On May 29, 2015, 8:44 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34829/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 8:44 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
>
>
> Bugs: DRILL-3190
> https://issues.apache.org/jira/browse/DRILL-3190
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3190: Check for transition from CANCELLATION_REQUESTED to non-terminal state in FragmentData
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
>
> Diff: https://reviews.apache.org/r/34829/diff/
>
>
> Testing
> -------
>
> Running unit and regresssion tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34829: DRILL-3190: Invalid FragmentState
transition from CANCELLATION_REQUESTED in QueryManager
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review86016
-----------------------------------------------------------
Ship it (non-binding)
- Chris Westin
On June 1, 2015, 10:54 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34829/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 10:54 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
>
>
> Bugs: DRILL-3190
> https://issues.apache.org/jira/browse/DRILL-3190
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3190: Check for transitions from CANCELLATION_REQUESTED state
> + Moved state transition checks to QueryManager
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 71b77c6
>
> Diff: https://reviews.apache.org/r/34829/diff/
>
>
> Testing
> -------
>
> Successful unit and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34829: DRILL-3190: Invalid FragmentState
transition from CANCELLATION_REQUESTED in QueryManager
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review86032
-----------------------------------------------------------
Ship it!
Ship It!
- abdelhakim deneche
On June 1, 2015, 5:54 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34829/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:54 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
>
>
> Bugs: DRILL-3190
> https://issues.apache.org/jira/browse/DRILL-3190
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3190: Check for transitions from CANCELLATION_REQUESTED state
> + Moved state transition checks to QueryManager
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 71b77c6
>
> Diff: https://reviews.apache.org/r/34829/diff/
>
>
> Testing
> -------
>
> Successful unit and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34829: DRILL-3190: Invalid FragmentState
transition from CANCELLATION_REQUESTED in QueryManager
Posted by Venki Korukanti <ve...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review86795
-----------------------------------------------------------
Ship it!
Ship It!
- Venki Korukanti
On June 1, 2015, 10:54 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34829/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 10:54 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
>
>
> Bugs: DRILL-3190
> https://issues.apache.org/jira/browse/DRILL-3190
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3190: Check for transitions from CANCELLATION_REQUESTED state
> + Moved state transition checks to QueryManager
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 71b77c6
>
> Diff: https://reviews.apache.org/r/34829/diff/
>
>
> Testing
> -------
>
> Successful unit and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34829: DRILL-3190: Invalid FragmentState
transition from CANCELLATION_REQUESTED in QueryManager
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/
-----------------------------------------------------------
(Updated June 1, 2015, 5:54 p.m.)
Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
Changes
-------
Addressed review comments
+ move fragment state transition logic to QueryManager
Bugs: DRILL-3190
https://issues.apache.org/jira/browse/DRILL-3190
Repository: drill-git
Description (updated)
-------
DRILL-3190: Check for transitions from CANCELLATION_REQUESTED state
+ Moved state transition checks to QueryManager
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 71b77c6
Diff: https://reviews.apache.org/r/34829/diff/
Testing (updated)
-------
Successful unit and regression tests.
Thanks,
Sudheesh Katkam
Re: Review Request 34829: DRILL-3190: Invalid FragmentState
transition from CANCELLATION_REQUESTED in QueryManager
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review85819
-----------------------------------------------------------
One minor optimization, then SHIP IT (non-binding)
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
<https://reviews.apache.org/r/34829/#comment137641>
This variable isn't used until later, so move it to after the if block so it isn't needlessly computed.
- Chris Westin
On May 29, 2015, 1:44 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34829/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 1:44 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Chris Westin, and Jacques Nadeau.
>
>
> Bugs: DRILL-3190
> https://issues.apache.org/jira/browse/DRILL-3190
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-3190: Check for transition from CANCELLATION_REQUESTED to non-terminal state in FragmentData
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java ceb77f0
>
> Diff: https://reviews.apache.org/r/34829/diff/
>
>
> Testing
> -------
>
> Running unit and regresssion tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>