[BEAM-596] Support waitUntilFinish, cancel in the DirectRunner by tgroh · Pull Request #1115 · apache/beam

tgroh · 2016-10-17T18:26:15Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

tgroh · 2016-10-17T20:26:22Z

Jenkins failure is unrelated (in spark)

tgroh · 2016-10-17T20:26:29Z

R: @peihe

peihe · 2016-10-17T20:58:08Z

runners/direct-java/src/main/java/org/apache/beam/runners/direct/PipelineExecutor.java

   *                   waiting thread and rethrows it
   */
-  void awaitCompletion() throws Exception;
+  State awaitCompletion(Duration duration) throws Exception;


throw checked exception?

peihe · 2016-10-17T20:58:21Z

runners/direct-java/src/main/java/org/apache/beam/runners/direct/PipelineExecutor.java

   *                   waiting thread and rethrows it
   */
-  void awaitCompletion() throws Exception;
+  State awaitCompletion(Duration duration) throws Exception;


Rename to waitUntilFinish?

peihe · 2016-10-18T01:32:50Z

...irect-java/src/main/java/org/apache/beam/runners/direct/ExecutorServiceParallelExecutor.java

-    } while (update == null || !update.isDone());
+    }
+    if (update == null || update.getNewState() == null) {
+      return executorService.isShutdown() ? State.UNKNOWN : State.RUNNING;


Since we control the executorService, could we make sure it provides the state when it is shutting down?

peihe · 2016-10-18T01:34:00Z

runners/direct-java/src/main/java/org/apache/beam/runners/direct/DirectRunner.java

+    @Override
+    public State cancel() {
+      executor.stop();
+      return waitUntilFinish();


any reason that the blocking logic is not in stop()?

This logic shouldn't really be present at all - this dispatches a call to the executor to shut it down, and then returns immediately. If the proper behavior includes waiting to finish, the call to waitToFinish should be explicit.

stop will stop the executor from starting any more work, but need not immediately shut down any in-progress work (but should work towards turning down any active work)

tgroh

This has been expanded slightly, and has been made much more resilient.

Additionally, the performance regression that it introduced has been mitigated. The call to BlockingQueue#poll(Timeout) appears to wait for the entire timeout duration if there is no element when initially called, which causes the Pipeline to wait for an extended period of time if the pipeline completes very quickly.

tgroh · 2016-12-22T23:14:56Z

runners/direct-java/src/main/java/org/apache/beam/runners/direct/DirectRunner.java

+    @Override
+    public State cancel() {
+      executor.stop();
+      return waitUntilFinish();


This logic shouldn't really be present at all - this dispatches a call to the executor to shut it down, and then returns immediately. If the proper behavior includes waiting to finish, the call to waitToFinish should be explicit.

stop will stop the executor from starting any more work, but need not immediately shut down any in-progress work (but should work towards turning down any active work)

tgroh · 2016-12-22T23:41:14Z

...irect-java/src/main/java/org/apache/beam/runners/direct/ExecutorServiceParallelExecutor.java

-    } while (update == null || !update.isDone());
+    }
+    if (update == null || update.getNewState() == null) {
+      return executorService.isShutdown() ? State.UNKNOWN : State.RUNNING;


asfbot · 2016-12-22T23:46:34Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6234/

Build result: FAILURE

[...truncated 5549 lines...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:332) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoFailureException: You have 1 Checkstyle violation. at org.apache.maven.plugin.checkstyle.CheckstyleViolationCheckMojo.execute(CheckstyleViolationCheckMojo.java:588) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2016-12-22T23:46:28.577 [ERROR] 2016-12-22T23:46:28.577 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2016-12-22T23:46:28.577 [ERROR] 2016-12-22T23:46:28.577 [ERROR] For more information about the errors and possible solutions, please read the following articles:2016-12-22T23:46:28.577 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2016-12-22T23:46:28.577 [ERROR] 2016-12-22T23:46:28.577 [ERROR] After correcting the problems, you can resume the build with the command2016-12-22T23:46:28.577 [ERROR] mvn -rf :beam-runners-direct-javachannel stoppedSetting status of f64ed31 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6234/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

asfbot · 2016-12-22T23:47:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6232/

Build result: FAILURE

[...truncated 5548 lines...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:332) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoFailureException: You have 1 Checkstyle violation. at org.apache.maven.plugin.checkstyle.CheckstyleViolationCheckMojo.execute(CheckstyleViolationCheckMojo.java:588) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2016-12-22T23:47:16.325 [ERROR] 2016-12-22T23:47:16.326 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2016-12-22T23:47:16.326 [ERROR] 2016-12-22T23:47:16.326 [ERROR] For more information about the errors and possible solutions, please read the following articles:2016-12-22T23:47:16.326 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2016-12-22T23:47:16.326 [ERROR] 2016-12-22T23:47:16.327 [ERROR] After correcting the problems, you can resume the build with the command2016-12-22T23:47:16.327 [ERROR] mvn -rf :beam-runners-direct-javachannel stoppedSetting status of 475eb11 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6232/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

asfbot · 2016-12-23T00:41:20Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6238/

Build result: FAILURE

[...truncated 5579 lines...] at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423) at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:77) at org.codehaus.groovy.reflection.CachedConstructor.doConstructorInvoke(CachedConstructor.java:71) at org.codehaus.groovy.runtime.callsite.ConstructorSite$ConstructorSiteNoUnwrap.callConstructor(ConstructorSite.java:81) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallConstructor(CallSiteArray.java:57) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:230) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:242) at org.codehaus.mojo.findbugs.FindbugsViolationCheckMojo.execute(FindbugsViolationCheckMojo.groovy:530) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2016-12-23T00:41:14.387 [ERROR] 2016-12-23T00:41:14.387 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2016-12-23T00:41:14.387 [ERROR] 2016-12-23T00:41:14.387 [ERROR] For more information about the errors and possible solutions, please read the following articles:2016-12-23T00:41:14.387 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2016-12-23T00:41:14.387 [ERROR] 2016-12-23T00:41:14.387 [ERROR] After correcting the problems, you can resume the build with the command2016-12-23T00:41:14.387 [ERROR] mvn -rf :beam-runners-direct-javachannel stoppedSetting status of a200c72 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6238/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

asfbot · 2016-12-23T01:16:03Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6239/
--none--

tgroh · 2017-02-06T19:01:28Z

@peihe This is ready to review again.

coveralls · 2017-02-06T19:47:14Z

Coverage increased (+0.03%) to 69.72% when pulling 62b46cf on tgroh:wait_until_finsish into d18db93 on apache:master.

asfbot · 2017-02-06T19:47:43Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7114/
--none--

coveralls · 2017-02-16T17:29:27Z

Coverage increased (+0.06%) to 69.339% when pulling 703e4ea on tgroh:wait_until_finsish into cd4e6e4 on apache:master.

asfbot · 2017-02-16T17:30:23Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7470/
--none--

davorbonaci · 2017-02-27T18:22:18Z

Any updates?

tgroh · 2017-02-27T19:16:40Z

No updates from my end. I'm happy to merge as-is or reassign reviewers.

aaltay · 2017-03-16T17:02:16Z

R: @kennknowles

kennknowles · 2017-03-22T16:24:45Z

LGTM

coveralls · 2017-03-22T17:36:38Z

Coverage increased (+0.05%) to 69.95% when pulling caacf29 on tgroh:wait_until_finsish into 2d9bf27 on apache:master.

asfbot · 2017-03-22T17:48:30Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8663/
--none--

peihe reviewed Oct 18, 2016

View reviewed changes

tgroh force-pushed the wait_until_finsish branch 2 times, most recently from 475eb11 to f64ed31 Compare December 22, 2016 23:40

tgroh commented Dec 22, 2016

View reviewed changes

tgroh force-pushed the wait_until_finsish branch 2 times, most recently from a200c72 to be9a1ec Compare December 23, 2016 00:37

tgroh force-pushed the wait_until_finsish branch from be9a1ec to 62b46cf Compare February 6, 2017 18:58

tgroh force-pushed the wait_until_finsish branch from 62b46cf to 703e4ea Compare February 16, 2017 16:57

Support waitUntilFinish, cancel in the DirectRunner

caacf29

tgroh force-pushed the wait_until_finsish branch from 703e4ea to caacf29 Compare March 22, 2017 16:49

asfgit merged commit caacf29 into apache:master Mar 22, 2017

asfgit pushed a commit that referenced this pull request Mar 22, 2017

This closes #1115

752a4c9

Comments

Conversation

tgroh commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgroh commented Oct 17, 2016

Uh oh!

tgroh commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgroh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Dec 22, 2016

Build result: FAILURE

Uh oh!

asfbot commented Dec 22, 2016

Build result: FAILURE

Uh oh!

asfbot commented Dec 23, 2016

Build result: FAILURE

Uh oh!

asfbot commented Dec 23, 2016

Uh oh!

tgroh commented Feb 6, 2017

Uh oh!

coveralls commented Feb 6, 2017

Uh oh!

asfbot commented Feb 6, 2017

Uh oh!

coveralls commented Feb 16, 2017

Uh oh!

asfbot commented Feb 16, 2017

Uh oh!

davorbonaci commented Feb 27, 2017

Uh oh!

tgroh commented Feb 27, 2017

Uh oh!

aaltay commented Mar 16, 2017

Uh oh!

kennknowles commented Mar 22, 2017

Uh oh!

coveralls commented Mar 22, 2017

Uh oh!

asfbot commented Mar 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tgroh commented Oct 17, 2016 •

edited

Loading