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/02 03:09:56 UTC
Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/
-----------------------------------------------------------
Review request for drill, Chris Westin and Jacques Nadeau.
Repository: drill-git
Description
-------
DRILL-2697: Pauses sites wait indefinitely for a resume signal
DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
+ Fix for bug in FragmentExecutor#closeOutResources
+ Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
+ Added execution controls to operator context
+ Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ae0f580
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java ccafa67
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java b7ffbf0
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
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/batch/ControlMessageHandler.java c5d78cc
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java edbcfde
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/FragmentExecutor.java 0783fee
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 508b10c
protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 813d961
protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
protocol/src/main/protobuf/BitControl.proto 0424725
protocol/src/main/protobuf/User.proto 59e22ae
Diff: https://reviews.apache.org/r/33770/diff/
Testing
-------
Passes all unit tests.
Thanks,
Sudheesh Katkam
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java, line 142
> > <https://reviews.apache.org/r/33770/diff/1/?file=947645#file947645line142>
> >
> > Does this mean that I can't have multiple pauses in the execution thread that will all work for a single query? For example, suppose I inject two pauses at different phases of execution: one just after planning and remote fragments are set up, and another after the first batch of results are returned. Will they both work, or will only the first one work?
>
> Sudheesh Katkam wrote:
> Yes, you can't. They will both be unpaused.
>
> I will re-submit a patch with client that unpauses a list of pause sites.
Will file a JIRA as an enhancement
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review82573
-----------------------------------------------------------
On May 8, 2015, 5:13 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 5:13 p.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
> + Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 5b28f16
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> 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/FragmentExecutor.java ddb828c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 93bc33c
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > A few minor things, along with some questions about resumeAll and the implications of that. I may have more suggestions based on the answers to my questions. Some of those might be best answered in the form of comments in the code or class javadoc because it seems likely others will have the same questions later.
I have some comments already but I will added a detailed explanation in PauseInjection class.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 183
> > <https://reviews.apache.org/r/33770/diff/1/?file=947643#file947643line183>
> >
> > Should this wait on acceptExternalEvents, in the same way cancel() does? What happens if we're not completely set up and we execute these? The queryContext one seems like it might be ok, but the queryManager one definitely seems like it could be a problem if this comes in before all the remote fragments are set up.
Right before the "foreman-ready" pause site, Foreman checks to make sure that if resume was called before it is completely setup that it calls resume again. The method is safe to call anytime and multiple times. If resume was called before Foreman is setup, the queryManager does not signal any fragments because there are no fragments in fragmentDataSet.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 184
> > <https://reviews.apache.org/r/33770/diff/1/?file=947643#file947643line184>
> >
> > All of them? I thought we were going to control this by queryId?
Execution controls lives within FragmentContext and QueryContext of a specific query; so this resume is for all pauses within current query context.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 264
> > <https://reviews.apache.org/r/33770/diff/1/?file=947643#file947643line264>
> >
> > Should you clear the resume flag after this?
No, the resume flag is to check if resume was called before Foreman is setup. It can be set to true once and remains true (since all pauses are resumed).
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java, line 404
> > <https://reviews.apache.org/r/33770/diff/1/?file=947650#file947650line404>
> >
> > I couldn't see why you need to use a Pointer<> here instead of just the Exception.
If an exception happens in the CancellingThread, the test does not fail. This ensures such an exception is caught.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java, line 633
> > <https://reviews.apache.org/r/33770/diff/1/?file=947650#file947650line633>
> >
> > Why not use assertTrue() here?
If the assert fails, the exception happens in the RPC thread and the test just hangs. The check method has a workaround for that to make the test fail.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java, line 660
> > <https://reviews.apache.org/r/33770/diff/1/?file=947650#file947650line660>
> >
> > assertTrue()?
^ same as above.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java, line 52
> > <https://reviews.apache.org/r/33770/diff/1/?file=947646#file947646line52>
> >
> > How about some javadoc explaining what this means? From this it's not at all obvious that this is about injected pauses instead of something like suspend/resume.
> >
> > Would "unpause()" be a better name?
I think unpause is a better name.
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review82573
-----------------------------------------------------------
On May 2, 2015, 1:09 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 1:09 a.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2697: Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Fix for bug in FragmentExecutor#closeOutResources
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ae0f580
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java ccafa67
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java b7ffbf0
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java edbcfde
> 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/FragmentExecutor.java 0783fee
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 508b10c
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 813d961
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 0424725
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java, line 58
> > <https://reviews.apache.org/r/33770/diff/1/?file=947639#file947639line58>
> >
> > I'm not sure about this waiting uninterruptibly. We're going to start using Thread.interrupt() to regain control of threads that are blocked waiting on queues or sockets if the fragment they are running on behalf of is cancelled. Seems like this should be handled in the same way. I can talk to you about that more, and Venki can tell you about what he's doing in this area.
This should not be a problem since this feature is used for testing (in a controlled environment). I feel it will in fact expose bugs; if not, we can remove this in the future.
> On May 5, 2015, 9:31 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java, line 142
> > <https://reviews.apache.org/r/33770/diff/1/?file=947645#file947645line142>
> >
> > Does this mean that I can't have multiple pauses in the execution thread that will all work for a single query? For example, suppose I inject two pauses at different phases of execution: one just after planning and remote fragments are set up, and another after the first batch of results are returned. Will they both work, or will only the first one work?
Yes, you can't. They will both be unpaused.
I will re-submit a patch with client that unpauses a list of pause sites.
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review82573
-----------------------------------------------------------
On May 2, 2015, 1:09 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 1:09 a.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2697: Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Fix for bug in FragmentExecutor#closeOutResources
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ae0f580
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java ccafa67
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java b7ffbf0
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java edbcfde
> 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/FragmentExecutor.java 0783fee
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 508b10c
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 813d961
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 0424725
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review82573
-----------------------------------------------------------
A few minor things, along with some questions about resumeAll and the implications of that. I may have more suggestions based on the answers to my questions. Some of those might be best answered in the form of comments in the code or class javadoc because it seems likely others will have the same questions later.
exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java
<https://reviews.apache.org/r/33770/#comment133305>
I'm not sure about this waiting uninterruptibly. We're going to start using Thread.interrupt() to regain control of threads that are blocked waiting on queues or sockets if the fragment they are running on behalf of is cancelled. Seems like this should be handled in the same way. I can talk to you about that more, and Venki can tell you about what he's doing in this area.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
<https://reviews.apache.org/r/33770/#comment133310>
Should this wait on acceptExternalEvents, in the same way cancel() does? What happens if we're not completely set up and we execute these? The queryContext one seems like it might be ok, but the queryManager one definitely seems like it could be a problem if this comes in before all the remote fragments are set up.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
<https://reviews.apache.org/r/33770/#comment133307>
All of them? I thought we were going to control this by queryId?
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
<https://reviews.apache.org/r/33770/#comment133309>
Should you clear the resume flag after this?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
<https://reviews.apache.org/r/33770/#comment133316>
Does this mean that I can't have multiple pauses in the execution thread that will all work for a single query? For example, suppose I inject two pauses at different phases of execution: one just after planning and remote fragments are set up, and another after the first batch of results are returned. Will they both work, or will only the first one work?
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java
<https://reviews.apache.org/r/33770/#comment133319>
How about some javadoc explaining what this means? From this it's not at all obvious that this is about injected pauses instead of something like suspend/resume.
Would "unpause()" be a better name?
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
<https://reviews.apache.org/r/33770/#comment133322>
I couldn't see why you need to use a Pointer<> here instead of just the Exception.
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
<https://reviews.apache.org/r/33770/#comment133323>
Instead of "TC 1", "TC 2", etc, can you please just copy-paste the one-line descriptions of these test cases from the original. Much easier if it's all described here inline.
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
<https://reviews.apache.org/r/33770/#comment133324>
Why not use assertTrue() here?
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
<https://reviews.apache.org/r/33770/#comment133325>
assertTrue()?
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
<https://reviews.apache.org/r/33770/#comment133327>
break the line at .setUserName() so only one attribute is set per line.
protocol/src/main/protobuf/BitControl.proto
<https://reviews.apache.org/r/33770/#comment133329>
Can we clean up this whitespace, and leave just a newline?
- Chris Westin
On May 1, 2015, 6:09 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 6:09 p.m.)
>
>
> Review request for drill, Chris Westin and Jacques Nadeau.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2697: Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Fix for bug in FragmentExecutor#closeOutResources
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ae0f580
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java ccafa67
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java b7ffbf0
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java edbcfde
> 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/FragmentExecutor.java 0783fee
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 508b10c
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 813d961
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 0424725
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review83159
-----------------------------------------------------------
Ship it (non-binding)
- Chris Westin
On May 9, 2015, 10:48 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 9, 2015, 10:48 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
> + Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 136d8c7
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java f92bb49
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java ae728d8
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java b3b7ae9
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java b7ef584
> 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/FragmentExecutor.java ddb828c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c3ff58b
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 93bc33c
> protocol/src/main/protobuf/User.proto 185a646
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/
-----------------------------------------------------------
(Updated May 9, 2015, 5:48 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
Changes
-------
Rebase on master (resolved merge conflicts)
Repository: drill-git
Description
-------
[DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
+ Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
+ Added execution controls to operator context
+ Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
+ Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 136d8c7
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java f92bb49
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java ae728d8
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java b3b7ae9
exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
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/batch/ControlMessageHandler.java c5d78cc
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java b7ef584
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/FragmentExecutor.java ddb828c
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c3ff58b
protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
protocol/src/main/protobuf/BitControl.proto 93bc33c
protocol/src/main/protobuf/User.proto 185a646
Diff: https://reviews.apache.org/r/33770/diff/
Testing
-------
Passes all unit tests.
Thanks,
Sudheesh Katkam
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review83124
-----------------------------------------------------------
Ship it (non-binding)
- Chris Westin
On May 8, 2015, 6:24 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 6:24 p.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
> + Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 9924704
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> 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/FragmentExecutor.java ddb828c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 93bc33c
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/
-----------------------------------------------------------
(Updated May 9, 2015, 1:24 a.m.)
Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
Changes
-------
Addressed review comments
Repository: drill-git
Description
-------
[DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
+ Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
+ Added execution controls to operator context
+ Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
+ Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 9924704
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
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/batch/ControlMessageHandler.java c5d78cc
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/FragmentExecutor.java ddb828c
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
protocol/src/main/protobuf/BitControl.proto 93bc33c
protocol/src/main/protobuf/User.proto 59e22ae
Diff: https://reviews.apache.org/r/33770/diff/
Testing
-------
Passes all unit tests.
Thanks,
Sudheesh Katkam
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 8, 2015, 6:36 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java, line 62
> > <https://reviews.apache.org/r/33770/diff/1-2/?file=947638#file947638line62>
> >
> > An abstract class without any methods? What's that about?
So instances are not created. It's an annotated class that is used only by Jackson's ObjectMapper to allow a list of injections to hold various types of injections (pause, latch, exception).
> On May 8, 2015, 6:36 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java, line 40
> > <https://reviews.apache.org/r/33770/diff/2/?file=953183#file953183line40>
> >
> > Can we make this constructor call the other one?
Not as part of this patch, how the allcator is assigned needs to be changed.
> On May 8, 2015, 6:36 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java, line 61
> > <https://reviews.apache.org/r/33770/diff/2/?file=953191#file953191line61>
> >
> > We really don't have to worry about anything here? Or should this wait (uninterruptibly) in a loop. Please add comments explaining the rationale for doing nothing.
> >
> > See http://www.ibm.com/developerworks/library/j-jtp05236/
Depends on the use case. I'll add both methods, and also add InterruptedException to the signature for await().
> On May 8, 2015, 6:36 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java, line 51
> > <https://reviews.apache.org/r/33770/diff/2/?file=953195#file953195line51>
> >
> > Since this is inside the NoOpControlsInjector class, you can use a shorter name, such as INJECTOR, or something like that, because the references will already be NoOpControlsInjector.INJECTOR.
> >
> > Also, is there any reason to make the instance public, since it is returned by getLatch()? Does anyone else need to get their hands on it?
Yes, in ExecutionControls#lookupCountDownLatchInjection. When there is no injection found, we return a latch that does nothing so that code does not throw NPE.
> On May 8, 2015, 6:36 p.m., Chris Westin wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java, line 109
> > <https://reviews.apache.org/r/33770/diff/2/?file=953208#file953208line109>
> >
> > Do we really have stuff that spawns threads like this? I thought most everything submitted Runnables to an executor service, which is slightly different. I don't think it affects anything here, but you might mention it in a comment re how this test works.
Yes, in PartitionSender.
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review83023
-----------------------------------------------------------
On May 8, 2015, 5:56 p.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 5:56 p.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
> + Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 5b28f16
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> 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/FragmentExecutor.java ddb828c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 93bc33c
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Chris Westin <ch...@gmail.com>.
> On May 8, 2015, 11:36 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java, line 62
> > <https://reviews.apache.org/r/33770/diff/1-2/?file=947638#file947638line62>
> >
> > An abstract class without any methods? What's that about?
>
> Sudheesh Katkam wrote:
> So instances are not created. It's an annotated class that is used only by Jackson's ObjectMapper to allow a list of injections to hold various types of injections (pause, latch, exception).
OK, please put some comments there at the class definition so that the next person to come along doesn't ask the same question...
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review83023
-----------------------------------------------------------
On May 8, 2015, 10:56 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 10:56 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
> + Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 5b28f16
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> 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/FragmentExecutor.java ddb828c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 93bc33c
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review83023
-----------------------------------------------------------
One bug, and some minor improvements.
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
<https://reviews.apache.org/r/33770/#comment133901>
An abstract class without any methods? What's that about?
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
<https://reviews.apache.org/r/33770/#comment133902>
=> "a countdown latch"
=> "injector, site description, and endpoint"
exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java
<https://reviews.apache.org/r/33770/#comment133905>
=> "each of which unpauses"
exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
<https://reviews.apache.org/r/33770/#comment133906>
Thanks for fixing this typo. It makes dealing with the code so much better for searching and reading.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java
<https://reviews.apache.org/r/33770/#comment133894>
Can we make this constructor call the other one?
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java
<https://reviews.apache.org/r/33770/#comment133899>
How about some javadoc for these methods?
There's no need to use "public" on methods on an interface -- they're public because they're on an interface.
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
<https://reviews.apache.org/r/33770/#comment133896>
You're missing an argument for the string format.
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
<https://reviews.apache.org/r/33770/#comment133897>
We really don't have to worry about anything here? Or should this wait (uninterruptibly) in a loop. Please add comments explaining the rationale for doing nothing.
See http://www.ibm.com/developerworks/library/j-jtp05236/
exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java
<https://reviews.apache.org/r/33770/#comment133904>
Since this is inside the NoOpControlsInjector class, you can use a shorter name, such as INJECTOR, or something like that, because the references will already be NoOpControlsInjector.INJECTOR.
Also, is there any reason to make the instance public, since it is returned by getLatch()? Does anyone else need to get their hands on it?
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
<https://reviews.apache.org/r/33770/#comment133915>
Do we really have stuff that spawns threads like this? I thought most everything submitted Runnables to an executor service, which is slightly different. I don't think it affects anything here, but you might mention it in a comment re how this test works.
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
<https://reviews.apache.org/r/33770/#comment133912>
yes, "and the test timeout mechanism will catch that case"
- Chris Westin
On May 8, 2015, 10:56 a.m., Sudheesh Katkam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33770/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 10:56 a.m.)
>
>
> Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
> DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
>
> + Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
> + Added execution controls to operator context
> + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
> + Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 5b28f16
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
> exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
> exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
> 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/batch/ControlMessageHandler.java c5d78cc
> 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/FragmentExecutor.java ddb828c
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
> exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
> protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
> protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
> protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
> protocol/src/main/protobuf/BitControl.proto 93bc33c
> protocol/src/main/protobuf/User.proto 59e22ae
>
> Diff: https://reviews.apache.org/r/33770/diff/
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Sudheesh Katkam
>
>
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/
-----------------------------------------------------------
(Updated May 8, 2015, 5:56 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
Changes
-------
Fixed a preconditions check bug.
Repository: drill-git
Description
-------
[DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
+ Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
+ Added execution controls to operator context
+ Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
+ Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 5b28f16
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
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/batch/ControlMessageHandler.java c5d78cc
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/FragmentExecutor.java ddb828c
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
protocol/src/main/protobuf/BitControl.proto 93bc33c
protocol/src/main/protobuf/User.proto 59e22ae
Diff: https://reviews.apache.org/r/33770/diff/
Testing
-------
Passes all unit tests.
Thanks,
Sudheesh Katkam
Re: Review Request 33770: DRILL-2697 Pause injections should pause
indefinitely until signalled
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/
-----------------------------------------------------------
(Updated May 8, 2015, 5:13 p.m.)
Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
Changes
-------
+ Rebased on master
+ Addressed Chris' comment
+ Added CountDownLatchInjection
Repository: drill-git
Description (updated)
-------
[DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites wait indefinitely for a resume signal
DrillClient sends a resume signal to UserServer. UserServer triggers a resume call in the correct Foreman. Foreman resumes all pauses related to the query through the Control layer.
+ Better error messages and more tests in TestDrillbitResilience and TestPauseInjection
+ Added execution controls to operator context
+ Removed ControlMessageHandler interface, renamed ControlHandlerImpl to ControlMessageHandler
+ Added CountDownLatchInjection, useful in cases like ParititionedSender that spawns multiple threads
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 5b28f16
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 7cc52ba
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java 6dbd880
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java 5b4d7bd
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java 37730e3
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java a4f9fdf
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java 88592d4
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 9e929de
exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java cf98b83
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java 1171bf8
exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java 4b1cd0c
exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 96fed3a
exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java 80d9790
exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java e5f9c9c
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/batch/ControlMessageHandler.java c5d78cc
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/FragmentExecutor.java ddb828c
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java 0ba91b4
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java f526fbe
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java b1c3fe0
exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 8854ef3
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java da69e9e
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java 5fa2b3f
protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976
protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47
protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 4d03073
protocol/src/main/protobuf/BitControl.proto 93bc33c
protocol/src/main/protobuf/User.proto 59e22ae
Diff: https://reviews.apache.org/r/33770/diff/
Testing
-------
Passes all unit tests.
Thanks,
Sudheesh Katkam