You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/03/31 15:52:03 UTC
[GitHub] [beam] regadas opened a new pull request #11275: [BEAM-9648]:
DirectRunner should return null on timeout
regadas opened a new pull request #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275
According to PipelineResult if waitUntilFinish(Duration) is supported it should return null.
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] regadas commented on a change in pull request #11275:
[BEAM-9648]: DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
regadas commented on a change in pull request #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#discussion_r405810674
##########
File path: runners/direct-java/src/main/java/org/apache/beam/runners/direct/ExecutorServiceParallelExecutor.java
##########
@@ -260,6 +260,11 @@ public State waitUntilFinish(Duration duration) throws Exception {
}
}
}
+
+ if (Instant.now().isAfter(completionTime)) {
+ return null;
+ }
Review comment:
@lukecwik well spotted I totally overlooked it! Thanks I'll update this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] regadas commented on a change in pull request #11275:
[BEAM-9648]: DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
regadas commented on a change in pull request #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#discussion_r405888827
##########
File path: runners/direct-java/src/main/java/org/apache/beam/runners/direct/ExecutorServiceParallelExecutor.java
##########
@@ -260,6 +260,11 @@ public State waitUntilFinish(Duration duration) throws Exception {
}
}
}
+
+ if (Instant.now().isAfter(completionTime)) {
+ return null;
+ }
Review comment:
Cleaned it up a little bit more and kept `(update == null && pipelineState.get().isTerminal())` to ensure that if an update has a `Throwable` it gets thrown. (keeps previous semantic).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] lukecwik merged pull request #11275: [BEAM-9648]:
DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
lukecwik merged pull request #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on a change in pull request #11275:
[BEAM-9648]: DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#discussion_r405807176
##########
File path: runners/direct-java/src/main/java/org/apache/beam/runners/direct/ExecutorServiceParallelExecutor.java
##########
@@ -260,6 +260,11 @@ public State waitUntilFinish(Duration duration) throws Exception {
}
}
}
+
+ if (Instant.now().isAfter(completionTime)) {
+ return null;
+ }
Review comment:
This allows for a race condition where we exit the while loop above not due to a timeout but would turn it into a timeout here because of the timing.
We could clean-up the loop above so it only exits on timeout while all non-timeout returns happen within the loop with something like:
```
while (Instant.now().isBefore(completionTime)) {
// Get an update; don't block forever if another thread has handled it. The call to poll will
// wait the entire timeout; this call primarily exists to relinquish any core.
VisibleExecutorUpdate update = visibleUpdates.tryNext(Duration.millis(25L));
if (pipelineState.get().isTerminal() || (update != null && isTerminalStateUpdate(update))) {
// there are no updates to process and no updates will ever be published because the
// executor is shutdown OR there has been an update and the update is terminal
return pipelineState.get();
} else if (update != null && update.thrown.isPresent()) {
Throwable thrown = update.thrown.get();
if (thrown instanceof Exception) {
throw (Exception) thrown;
} else if (thrown instanceof Error) {
throw (Error) thrown;
} else {
throw new Exception("Unknown Type of Throwable", thrown);
}
}
}
return null;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] regadas commented on issue #11275: [BEAM-9648]: DirectRunner
should return null on timeout
Posted by GitBox <gi...@apache.org>.
regadas commented on issue #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#issuecomment-607538427
R: @lukecwik
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] regadas commented on a change in pull request #11275:
[BEAM-9648]: DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
regadas commented on a change in pull request #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#discussion_r405889052
##########
File path: runners/direct-java/src/main/java/org/apache/beam/runners/direct/ExecutorServiceParallelExecutor.java
##########
@@ -269,7 +277,7 @@ public State getPipelineState() {
}
private boolean isTerminalStateUpdate(VisibleExecutorUpdate update) {
- return !(update.getNewState() == null && update.getNewState().isTerminal());
+ return update.getNewState() != null && update.getNewState().isTerminal();
Review comment:
@lukecwik also found out that this conditional would throw an NPE
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] regadas commented on a change in pull request #11275:
[BEAM-9648]: DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
regadas commented on a change in pull request #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#discussion_r405888827
##########
File path: runners/direct-java/src/main/java/org/apache/beam/runners/direct/ExecutorServiceParallelExecutor.java
##########
@@ -260,6 +260,11 @@ public State waitUntilFinish(Duration duration) throws Exception {
}
}
}
+
+ if (Instant.now().isAfter(completionTime)) {
+ return null;
+ }
Review comment:
Cleaned it up a little bit more and kept `(update == null && pipelineState.get().isTerminal())` to ensure that if an update has a `Throwable` it gets thrown. (keeps previous semantic).
Again thx for spotting the race condition 🤦♂
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11275: [BEAM-9648]:
DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#issuecomment-612983658
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11275: [BEAM-9648]:
DirectRunner should return null on timeout
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#issuecomment-612983443
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] iemejia commented on issue #11275: [BEAM-9648]: DirectRunner
should return null on timeout
Posted by GitBox <gi...@apache.org>.
iemejia commented on issue #11275: [BEAM-9648]: DirectRunner should return null on timeout
URL: https://github.com/apache/beam/pull/11275#issuecomment-610864265
Run Direct ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services