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/09 03:14:57 UTC
Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/
-----------------------------------------------------------
Review request for drill and Jacques Nadeau.
Repository: drill-git
Description
-------
**INITIAL PATCH**
The current implementation waits for the first batch to start a pending remote fragment. This wait is unnecessary given the fast schema behavior; start the fragment as soon as it receives the plan fragment from the Foreman. In this case startFragmentPendingRemote() and addFragmentRunner() are redundant in WorkManager.
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandlerImpl.java e18b94c
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/batch/ControlHandlerImpl.java b6c6852
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 49d0c94
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 34fa639
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
Diff: https://reviews.apache.org/r/34008/diff/
Testing
-------
Thanks,
Sudheesh Katkam
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83346
-----------------------------------------------------------
Ship it!
Ship It!
- Jacques Nadeau
On May 12, 2015, 12:32 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 12:32 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2977](https://issues.apache.org/jira/browse/DRILL-2977), [DRILL-2978](https://issues.apache.org/jira/browse/DRILL-2978): Swap fragment execution methods and cancellation changes
>
> Execution: In WorkManager,
> + swap implementations of startFragmentPendingRemote() and addFragmentRunner()
> + warn if there are running fragments in close()
>
> Cancellation:
> + for fragments waiting on data, delegate cancellations to WorkEventBus (in Foreman and ControlMessageHandler)
> + documentation
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java d90096a
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 1d3a0b0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java d12e6d5
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 090a377
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 67ef9b8
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83330
-----------------------------------------------------------
Ship it!
Ship It!
- abdelhakim deneche
On May 12, 2015, 12:32 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 12:32 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2977](https://issues.apache.org/jira/browse/DRILL-2977), [DRILL-2978](https://issues.apache.org/jira/browse/DRILL-2978): Swap fragment execution methods and cancellation changes
>
> Execution: In WorkManager,
> + swap implementations of startFragmentPendingRemote() and addFragmentRunner()
> + warn if there are running fragments in close()
>
> Cancellation:
> + for fragments waiting on data, delegate cancellations to WorkEventBus (in Foreman and ControlMessageHandler)
> + documentation
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java d90096a
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 1d3a0b0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java d12e6d5
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 090a377
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 67ef9b8
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/
-----------------------------------------------------------
(Updated May 12, 2015, 12:32 a.m.)
Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
Changes
-------
Addressed review comments
Repository: drill-git
Description
-------
[DRILL-2977](https://issues.apache.org/jira/browse/DRILL-2977), [DRILL-2978](https://issues.apache.org/jira/browse/DRILL-2978): Swap fragment execution methods and cancellation changes
Execution: In WorkManager,
+ swap implementations of startFragmentPendingRemote() and addFragmentRunner()
+ warn if there are running fragments in close()
Cancellation:
+ for fragments waiting on data, delegate cancellations to WorkEventBus (in Foreman and ControlMessageHandler)
+ documentation
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java d90096a
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 1d3a0b0
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java d12e6d5
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 090a377
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 67ef9b8
Diff: https://reviews.apache.org/r/34008/diff/
Testing
-------
Passes all unit tests and regression tests.
Thanks,
Sudheesh Katkam
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83316
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java
<https://reviews.apache.org/r/34008/#comment134254>
comment missing a word ? "The manager for the ? will ..."
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java
<https://reviews.apache.org/r/34008/#comment134255>
shouldn't we log a warning if the manager we are trying to remove is not in the map ?
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java
<https://reviews.apache.org/r/34008/#comment134257>
we should probably explain what the returned boolean mean
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java
<https://reviews.apache.org/r/34008/#comment134258>
logging the running fragments should be done at a debug level
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java
<https://reviews.apache.org/r/34008/#comment134260>
the fragment can also be a root fragment. Maybe just say "the manager for the fragment"
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
<https://reviews.apache.org/r/34008/#comment134262>
where is Case 1 ???
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
<https://reviews.apache.org/r/34008/#comment134267>
we don't need this change, see next comment
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
<https://reviews.apache.org/r/34008/#comment134266>
drillbitContext.getWorkBus() can be used to access the work event bus, so you don't need to pass the WorkerBee
- abdelhakim deneche
On May 11, 2015, 11:16 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 11, 2015, 11:16 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2977](https://issues.apache.org/jira/browse/DRILL-2977), [DRILL-2978](https://issues.apache.org/jira/browse/DRILL-2978): Swap fragment execution methods and cancellation changes
>
> Execution: In WorkManager,
> + swap implementations of startFragmentPendingRemote() and addFragmentRunner()
> + warn if there are running fragments in close()
>
> Cancellation:
> + for fragments waiting on data, delegate cancellations to WorkEventBus (in Foreman and ControlMessageHandler)
> + documentation
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java d90096a
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 1d3a0b0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java d12e6d5
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java bf62ccb
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 090a377
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 67ef9b8
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/
-----------------------------------------------------------
(Updated May 11, 2015, 11:16 p.m.)
Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
Changes
-------
Addressed Hakim's comments
+ includes DRILL-2977 and DRILL-2978
+ passes all tests
Repository: drill-git
Description (updated)
-------
[DRILL-2977](https://issues.apache.org/jira/browse/DRILL-2977), [DRILL-2978](https://issues.apache.org/jira/browse/DRILL-2978): Swap fragment execution methods and cancellation changes
Execution: In WorkManager,
+ swap implementations of startFragmentPendingRemote() and addFragmentRunner()
+ warn if there are running fragments in close()
Cancellation:
+ for fragments waiting on data, delegate cancellations to WorkEventBus (in Foreman and ControlMessageHandler)
+ documentation
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/WorkEventBus.java d90096a
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 1d3a0b0
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java d12e6d5
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java bf62ccb
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 090a377
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java 67ef9b8
Diff: https://reviews.apache.org/r/34008/diff/
Testing
-------
Passes all unit tests and regression tests.
Thanks,
Sudheesh Katkam
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 10, 2015, 7:55 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java, line 181
> > <https://reviews.apache.org/r/34008/diff/2/?file=955018#file955018line181>
> >
> > should we make this line a warning ?
Yes; I will make this change as part of DRILL-2978 (I was working on both patches together but I forgot to make the change as part of this patch).
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83189
-----------------------------------------------------------
On May 10, 2015, 7:13 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 10, 2015, 7:13 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> + Undid the initial patch
> + Only swap the implementations
> + Rebased on master [87051d4]
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 3e4f3d1
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83189
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java
<https://reviews.apache.org/r/34008/#comment134075>
should we make this line a warning ?
- abdelhakim deneche
On May 10, 2015, 7:13 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 10, 2015, 7:13 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> + Undid the initial patch
> + Only swap the implementations
> + Rebased on master [87051d4]
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 3e4f3d1
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 11, 2015, 12:45 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java, line 292
> > <https://reviews.apache.org/r/34008/diff/2/?file=955018#file955018line292>
> >
> > We should remove the fragment manager from the work bus if it's fragment executor has already been cancelled. Otherwise, it will stay in the work bus forever.
I will make this change as part of DRILL-2978.
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83221
-----------------------------------------------------------
On May 10, 2015, 7:13 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 10, 2015, 7:13 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> + Undid the initial patch
> + Only swap the implementations
> + Rebased on master [87051d4]
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 3e4f3d1
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83221
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java
<https://reviews.apache.org/r/34008/#comment134128>
We should remove the fragment manager from the work bus if it's fragment executor has already been cancelled. Otherwise, it will stay in the work bus forever.
- abdelhakim deneche
On May 10, 2015, 7:13 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 10, 2015, 7:13 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> + Undid the initial patch
> + Only swap the implementations
> + Rebased on master [87051d4]
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 3e4f3d1
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/#review83190
-----------------------------------------------------------
LGTM, +1 (non binding)
- abdelhakim deneche
On May 10, 2015, 7:13 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34008/
> -----------------------------------------------------------
>
> (Updated May 10, 2015, 7:13 p.m.)
>
>
> Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> + Undid the initial patch
> + Only swap the implementations
> + Rebased on master [87051d4]
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 3e4f3d1
>
> Diff: https://reviews.apache.org/r/34008/diff/
>
>
> Testing
> -------
>
> Passes all unit tests and regression tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/
-----------------------------------------------------------
(Updated May 10, 2015, 7:13 p.m.)
Review request for drill, abdelhakim deneche, Jacques Nadeau, and Venki Korukanti.
Repository: drill-git
Description
-------
+ Undid the initial patch
+ Only swap the implementations
+ Rebased on master [87051d4]
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 3e4f3d1
Diff: https://reviews.apache.org/r/34008/diff/
Testing (updated)
-------
Passes all unit tests and regression tests.
Thanks,
Sudheesh Katkam
Re: Review Request 34008: DRILL:2977: In WorkManager,
startFragmentPendingRemote() and addFragmentRunner() need to be permuted
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34008/
-----------------------------------------------------------
(Updated May 10, 2015, 6:30 p.m.)
Review request for drill and Jacques Nadeau.
Repository: drill-git
Description (updated)
-------
+ Undid the initial patch
+ Only swap the implementations
+ Rebased on master [87051d4]
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 3e4f3d1
Diff: https://reviews.apache.org/r/34008/diff/
Testing
-------
Thanks,
Sudheesh Katkam