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 2021/05/04 09:16:01 UTC
[GitHub] [beam] je-ik opened a new pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
je-ik opened a new pull request #14718:
URL: https://github.com/apache/beam/pull/14718
Fixes [BEAM-12276].
------------------------
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).
`ValidatesRunner` compliance status (on master branch)
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/badge/icon?subject=V1+Streaming">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon?subject=V1+Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/badge/icon?subject=V2+Streaming">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon?subject=Java+8">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon?subject=Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon?subject=Portable+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon?subject=Structured+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon?subject=ValCont">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
</tbody>
</table>
Examples testing status on various runners
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/badge/icon?subject=V1+Java11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
Post-Commit SDK/Transform Integration Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>Go</th>
<th>Java</th>
<th>Python</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon?subject=3.6">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon?subject=3.7">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon?subject=3.8">
</a>
</td>
</tr>
</tbody>
</table>
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>---</th>
<th>Java</th>
<th>Python</th>
<th>Go</th>
<th>Website</th>
<th>Whitespace</th>
<th>Typescript</th>
</tr>
</thead>
<tbody>
<tr>
<td>Non-portable</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon?subject=Tests">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon?subject=Lint">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon?subject=Docker">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon?subject=Docs">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Portable</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
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.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-833262473
@reuvenlax not sure from the last note, is this good to merge?
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847218537
There is still another issue - if we enforce the output timestamp to be within bounds of the window, we will block output watermark on the boundary of window, up to window GC time. That sounds incorrect. The case of timer set to time in the interval window.maxTimestamp() and (window.maxTimestamp() + allowedLateness) seems to be somewhat special and needs special handling. It should definitely not place watermark hold.
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-844390496
@boyuanzz can you help review this, please? Looks a little stuck.
--
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
[GitHub] [beam] kennknowles commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832090563
Nonetheless the check here doesn't have any basis in real requirements, right?
--
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
[GitHub] [beam] kennknowles commented on a change in pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #14718:
URL: https://github.com/apache/beam/pull/14718#discussion_r630459015
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/SimpleDoFnRunner.java
##########
@@ -1219,11 +1219,11 @@ private void setAndVerifyOutputTimestamp() {
Instant windowExpiry = window.maxTimestamp().plus(allowedLateness);
if (TimeDomain.EVENT_TIME.equals(spec.getTimeDomain())) {
checkArgument(
- !outputTimestamp.isAfter(target),
+ !outputTimestamp.isAfter(windowExpiry),
Review comment:
Actually now that I think about it, the output timestamp really should be before the end of the window. That is a requirement on `WindowFn.assign` and other timestamp adjusting methods.
--
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
[GitHub] [beam] reuvenlax commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
reuvenlax commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832134625
Disallowing the output timestamp being later than the input timestamp
doesn't make any sense -the input watermark is a lower bound, so will
usually be less than anything being processed. If that's what the code is
doing today, then it's clearly wrong.
On Tue, May 4, 2021 at 10:51 AM Jan Lukavský ***@***.***>
wrote:
> No, you can verify that by running the test without the changed
> precondition.
> I'm not sure if relative timer based ob element's timestamp makes sense to
> me. Using input watermark makes perfect analogy with relative processing
> time timers, at least as I understand the concept.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/14718#issuecomment-832128322>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFAYJVMQD26FJE5BSBGSJI3TMAXYXANCNFSM44CKPPKQ>
> .
>
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847239744
Thinking more about it, there are three cases:
1) setting timer with the scope of window => `timer.set(X)` should then have the semantics of `timer.withOutputTimestamp(X).set(X)`, watermark hold must be placed at instant X
2) setting timer out of the scope of window (within lateness) => `timer.set(X)` should have the the semantics of timer.withOutputTimestamp(endOfWindow).set(X)`, but *should not set the hold* (or should set it for the window GC time, which should be done by default
3) setting timer out of the scope of window (within lateness) => `timer.withOutputTimestamp(within window validity).set(windowGc)` should set the watermark hold, as that is the intent
Cases 2) and 3) show, that we must be able to distinguish the case when the user explicitly sets output timestamp and when he does not. I'm afraid these changes are too deep, therefore I'd suggest to again return to solution b) or d) above, close this PR and possibly do another follow up in different JIRA. This might probably require discussion on the ML.
--
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
[GitHub] [beam] boyuanzz commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-844398167
> @boyuanzz can you help review this, please? Looks a little stuck.
Yes, I'll review it later today.
--
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
[GitHub] [beam] reuvenlax commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
reuvenlax commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832108998
The fire timestamp for the timer should be the timestamp of the timer
itself, not the current input watermark. One way to get is is using
OnTimerContext.fireTimestamp. You can also get it by adding a @Timestamp
parameter to the onTimer function, which will be filled in using the fire
timestamp.
On Tue, May 4, 2021 at 10:17 AM Jan Lukavský ***@***.***>
wrote:
> @reuvenlax <https://github.com/reuvenlax> the problem is with relative
> timers. I'm not aware of a way to get the input watermark, which is the
> firing timestamp for relative event time timer with zero offset. Which is
> tested in the test case.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/14718#issuecomment-832106898>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFAYJVKCLW2M3WAUWK3NATLTMAT4HANCNFSM44CKPPKQ>
> .
>
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832123747
There is no fireTimestamp until the timer is actually fired. The use case sets the timer in @ProcessElement. `Timer.withOutputTimestamp().offset(Duration.ZERO).setRelative()` should fire as soon as watermark moves, the fireTimestamp of the set timer is the current value of the input watermark (presumably, I didn't investigate the code, but it makes sense). Whether or not this call throws exception depends on the current value of the input watermark (or the base for relative event time timers, may it be anything).
If you look at the test case I think there is nothing else left to do to make it succeed without changing the precondition check (or making input watermark available to the user somehow).
Another note is that setting watermarkhold to time T + delta is semantically a no-op, the output watermark cannot be ahead of the input watermark, so that doesn't break anything.
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-831923890
Run Java PreCommit
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847575729
The hold part might not be part of conceptual model, but the output timestamp has direct implications, that involve it. The reason is in the underlying invariant - if element arrives ahead of watermark, it should stay ahead of watermark throughout the whole computation, if possible (the same holds true for droppable data). Therefore, if a timer sets output timestamp for earlier timestamp than the fire timestamp and the output timestamp is ahead of output watermark, it simply implies watermark hold, otherwise the element might be output after the output watermark, causing it to be late (or even droppable).
When it comes to timers that are set for time interval between (endOfWindow, windowGcTime) the situation is different. There is no way to infer if there should, or should not be a watermark hold (assuming we enforce the output timestamp to be within the bounds of window), because the situation is exactly the same - fire timestamp for windowGc time, output timestamp endOfWindow. There has to be explicit user-code declarations of this. (e.g. `timer.withLateOutputTimestamp(...)`, as opposed to `timer.withOutputTimestamp`, which would have the semantics "withOnTimeOutputTimestamp").
Conclusion from my side - if we want to implement c) we need a deeper change to the timer API, it is definitely not about adding/removing/modifying a few preconditions, we should solve it in different JIRA. Please stop me, if my conclusions are wrong, otherwise I'll merge this tomorrow - just to make it explicit, this PR is the option b) from the options above.
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847215896
If we enforce the use of explicit output timestamp I think that from the correctness perspective, it really looks like c) is the way to go. I'm still not convinced we can simply ignore the fact that it is breaking change, though. Mostly because it is a *runtime* breaking change, that can result in production downtimes (if a buggy code is not revealed during testing of a pipeline, which might happen).
--
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
[GitHub] [beam] kennknowles commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832067257
I was digging through the history and it came from https://github.com/apache/beam/pull/11252/files. That change just seems wrong. BEAM-9557 seems to be about something else entirely.
--
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
[GitHub] [beam] steveniemitz commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847187730
`c` seems too strict? It seems like it should be valid to emit data between `[window.maxTimestamp, window.maxTimestamp + allowedLateness)`.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #14718:
URL: https://github.com/apache/beam/pull/14718#discussion_r630804066
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/SimpleDoFnRunner.java
##########
@@ -1219,11 +1219,11 @@ private void setAndVerifyOutputTimestamp() {
Instant windowExpiry = window.maxTimestamp().plus(allowedLateness);
if (TimeDomain.EVENT_TIME.equals(spec.getTimeDomain())) {
checkArgument(
- !outputTimestamp.isAfter(target),
+ !outputTimestamp.isAfter(windowExpiry),
Review comment:
Forcing the output timestamp to be before the end of the window is actually quite logical requirement, but is a breaking change. It will break code that sets timer (without output timestamp) to endOfWindow + lateness. Therefore I think we must keep the check as it is. Yes, it might result in data from a particular window being emitted for a different window (which really seems like a bug).
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832128322
No, you can verify that by running the test without the changed precondition.
I'm not sure if relative timer based ob element's timestamp makes sense to me. Using input watermark makes perfect analogy with relative processing time timers, at least as I understand the concept.
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-837984700
@kennknowles @reuvenlax I think we should get this into the 2.31.0 release, can we merge this? Or are there still any doubts?
--
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
[GitHub] [beam] je-ik merged pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik merged pull request #14718:
URL: https://github.com/apache/beam/pull/14718
--
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
[GitHub] [beam] je-ik commented on a change in pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #14718:
URL: https://github.com/apache/beam/pull/14718#discussion_r630804066
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/SimpleDoFnRunner.java
##########
@@ -1219,11 +1219,11 @@ private void setAndVerifyOutputTimestamp() {
Instant windowExpiry = window.maxTimestamp().plus(allowedLateness);
if (TimeDomain.EVENT_TIME.equals(spec.getTimeDomain())) {
checkArgument(
- !outputTimestamp.isAfter(target),
+ !outputTimestamp.isAfter(windowExpiry),
Review comment:
Require the output timestamp to be before the end of the window is actually quite logical requirement, but is a breaking change. It will break code that sets timer (without output timestamp) to endOfWindow + lateness. Therefore I think we must keep the check as it is. Yes, it might result in data from a particular window being emitted for a different window (which really seems like a bug).
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832106898
@reuvenlax the problem is with relative timers. I'm not aware of a way to get the input watermark, which is the firing timestamp for relative event time timer with zero offset. Which is tested in the test case.
--
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
[GitHub] [beam] reuvenlax commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
reuvenlax commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832120099
FYI, I'm not opposed to loosening this check. I just want to make sure I
understand the use case.
On Tue, May 4, 2021 at 10:20 AM Reuven Lax ***@***.***> wrote:
> The fire timestamp for the timer should be the timestamp of the timer
> itself, not the current input watermark. One way to get is is using
> OnTimerContext.fireTimestamp. You can also get it by adding a @Timestamp
> parameter to the onTimer function, which will be filled in using the fire
> timestamp.
>
> On Tue, May 4, 2021 at 10:17 AM Jan Lukavský ***@***.***>
> wrote:
>
>> @reuvenlax <https://github.com/reuvenlax> the problem is with relative
>> timers. I'm not aware of a way to get the input watermark, which is the
>> firing timestamp for relative event time timer with zero offset. Which is
>> tested in the test case.
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <https://github.com/apache/beam/pull/14718#issuecomment-832106898>, or
>> unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AFAYJVKCLW2M3WAUWK3NATLTMAT4HANCNFSM44CKPPKQ>
>> .
>>
>
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-846842691
@boyuanzz @reuvenlax @kennknowles I'd like to reach a consensus here. We have four options:
a) leave thing as they are
b) relax the check, so that it checks only that the output timestamp is not after window GC time
c) modify the check so that it checks the output timestamp is not after window.maxTimestamp
d) remove the check completely
I think a strong arguments were made that option a) is wrong. Option c) seems to be a breaking change to me - existing pipelines, that use withOutputTimestamp might break, because there is nothing stopping them (now) to use output timestamp that is higher than window.maxTimestamp. Therefore, I think we should rule option c) out as well.
This leaves options b) and d) - option b) is in this PR, option d) is simplistic modification. :-)
From my point of view, because it is legitimate to use `withOutputTimestamp` anytime an output is generated and there is nothing stopping the user to output the element arbitrarily far in the future, we should use the same logic and allow the same for timer output timestamps.
So, to conclude - I'm fine with both b) and d), while d) seems to be the most clean solution.
--
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
[GitHub] [beam] boyuanzz commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-846186615
Changes look good to me because it loosens existing conditions.
But I have a question regarding to `outputTimestamp` and `fireTimestamp`. If we set a `watermarkHold` that is larger than the `fireTimestamp`, won't the timer never be fired?
--
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
[GitHub] [beam] kennknowles commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847204036
it is a good point about `timer.set(window.maxTimestamp().plus(X))`. I believe we should then require to have a separate output timestamp like `timer.set(window.maxTimestamp().plus(X)).withOutputTimestamp(window.maxTimestamp())`.
--
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
[GitHub] [beam] kennknowles commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847247195
To be clear, I do not think that a "hold" is part of the user conceptual model and I do not think it should be added. It can be up to each runner how it wants to manage it. The purpose of the method is actually to control the event timestamp of outputs, and watermarks should be managed by the runner. But, yes, in practice they all use shared hold code.
I would say this:
1. setting timer with the scope of window => `timer.set(X)` callback produces outputs with timestamp `X` as a default.
2. setting timer out of the scope of window (within lateness) => user needs to specify output timestamp since the default does not make sense.
The runner, or runners-core can decide based on many things. For example in GBK the runners-core actually uses the current watermark value to decide how to set the hold. If the time of the hold has already passed, then we do some things to avoid pausing the watermark, just making sure the window does not expire.
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-846375992
> If we set a `watermarkHold` that is larger than the `fireTimestamp`, won't the timer never be fired?
No, because watermark hold affects the output watermark, while timers fire based upon the input watermark. So the hold should not affect firing of timers at all.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #14718:
URL: https://github.com/apache/beam/pull/14718#discussion_r632911653
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/SimpleDoFnRunner.java
##########
@@ -1219,11 +1219,11 @@ private void setAndVerifyOutputTimestamp() {
Instant windowExpiry = window.maxTimestamp().plus(allowedLateness);
if (TimeDomain.EVENT_TIME.equals(spec.getTimeDomain())) {
checkArgument(
- !outputTimestamp.isAfter(target),
+ !outputTimestamp.isAfter(windowExpiry),
Review comment:
GIving it one more thought, I think we should actually drop the Precondition for the output timestamp all together. Limiting it to any (future) timestamp seems inconsistent with the fact, that there is nothing stopping the user from using `outputWithTimestamp` and output arbitrarily future-bound value. The only thing the timer output timestamp actually influences is the output watermark. The output watermark is bound to be at most the value of input watermark, so setting a watermark hold to something higher is a no-op (semantically). Therefore this check should be removed completely.
@kennknowles @reuvenlax WDYT?
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/SimpleDoFnRunner.java
##########
@@ -1219,11 +1219,11 @@ private void setAndVerifyOutputTimestamp() {
Instant windowExpiry = window.maxTimestamp().plus(allowedLateness);
if (TimeDomain.EVENT_TIME.equals(spec.getTimeDomain())) {
checkArgument(
- !outputTimestamp.isAfter(target),
+ !outputTimestamp.isAfter(windowExpiry),
"Attempted to set an event-time timer with an output timestamp of %s that is"
- + " after the timer firing timestamp %s",
+ + " after the expiration of window %s",
outputTimestamp,
- target);
+ windowExpiry);
checkArgument(
!target.isAfter(windowExpiry),
Review comment:
Yes, agree.
--
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
[GitHub] [beam] kennknowles commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847203179
Taking a concrete example, suppose you have fixed windows of an hour. What (b) allows is to output an element with event timestamp 3:45pm in the window [2:00pm - 3:00pm). So naively, it is not logical to output at a timestamp where the `WindowFn` would not assign the window.
However, we do allow this elsewhere. You can do `Window.into(...)` and then immediately do a `ParDo` that moves the timestamps. I _think_ in this case you can move the timestamp past the window max but I do not recall without checking the code. So that is why I am OK with (b) or (d). Generally they are consistent with other things that you can do, even though they may not make sense.
I think (c) is the one that is obviously logical. It means that the element always actually falls within the window that it is assigned. That is sort of the point of windows, really.
I think SQL has the clearest interpretation: a window is just another thing from the GROUP BY that has some way of knowing when it is done. There is no concept of decoupling window and element timestamp - you just GROUP BY window. Once the window has been added to a row, the other values don't really matter. But the watermark needs to be faithful to the window.
--
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
[GitHub] [beam] je-ik commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847200605
c) has additional question about what the semantics of
```java
timer.set(window.maxTimestamp().plus(X))
```
should be. Should it be `timer.withOutputTimestamp(window.maxTimestamp()).set(window.maxTimestamp().plus(X))`? Although I think it is semantically correct, my concerns are really about the backwards compatibility. And as mentioned - given the fact that we do not restrict timestamp of `outputWithTimestamp`, I think we should leave it entirely on the user to emit data with timestamp for some (future) window.
--
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
[GitHub] [beam] reuvenlax commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
reuvenlax commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832125323
In that case, the check was intended to be based on the timestamp of the
element being processed by ProcessElement, not based on the input
watermark. Was this not the case?
On Tue, May 4, 2021 at 10:44 AM Jan Lukavský ***@***.***>
wrote:
> There is no fireTimestamp until the timer is actually fired. The use case
> sets the timer in @ProcessElement.
> Timer.withOutputTimestamp().offset(Duration.ZERO).setRelative() should
> fire as soon as watermark moves, the fireTimestamp of the set timer is the
> current value of the input watermark (presumably, I didn't investigate the
> code, but it makes sense). Whether or not this call throws exception
> depends on the current value of the input watermark (or the base for
> relative event time timers, may it be anything).
> If you look at the test case I think there is nothing else left to do to
> make it succeed without changing the precondition check (or making input
> watermark available to the user somehow).
> Another note is that setting watermarkhold to time T + delta is
> semantically a no-op, the output watermark cannot be ahead of the input
> watermark, so that doesn't break anything.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/14718#issuecomment-832123747>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFAYJVN6NRUYJKZ7TQ5IVUDTMAW6BANCNFSM44CKPPKQ>
> .
>
--
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
[GitHub] [beam] kennknowles commented on a change in pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #14718:
URL: https://github.com/apache/beam/pull/14718#discussion_r630459488
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/SimpleDoFnRunner.java
##########
@@ -1219,11 +1219,11 @@ private void setAndVerifyOutputTimestamp() {
Instant windowExpiry = window.maxTimestamp().plus(allowedLateness);
if (TimeDomain.EVENT_TIME.equals(spec.getTimeDomain())) {
checkArgument(
- !outputTimestamp.isAfter(target),
+ !outputTimestamp.isAfter(windowExpiry),
"Attempted to set an event-time timer with an output timestamp of %s that is"
- + " after the timer firing timestamp %s",
+ + " after the expiration of window %s",
outputTimestamp,
- target);
+ windowExpiry);
checkArgument(
!target.isAfter(windowExpiry),
Review comment:
This check is right: you shouldn't set an event time timer to fire after expiry, because it would be ignored.
--
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
[GitHub] [beam] reuvenlax commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
reuvenlax commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832094502
The general invariant for watermark-correct operators was that processing
of an element with timestamp T does not produce a timestamp > T. This check
was an attempt to extend that timer processing.
Making this a hard check is maybe too strict, but I'm still not sure I
understand the use case. Allowing future timestamps means that you have a
watermark hold for timestamps T+delta, but you remove the watermark hold
when the watermark reaches T. I guess we could allow it, but it still seems
odd to me.
On Tue, May 4, 2021 at 9:52 AM Kenn Knowles ***@***.***>
wrote:
> Nonetheless the check here doesn't have any basis in real requirements,
> right?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/14718#issuecomment-832090563>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFAYJVJYAUOUHL2ET4HD5FLTMAQ3PANCNFSM44CKPPKQ>
> .
>
--
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
[GitHub] [beam] kennknowles commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832070801
I think your change is correct. Essentially reverting the prior change, which broke things. On the bug it seems we found a bug in how cleanup timers were fired (for just Dataflow? for everyone?) and instead of fixing the bug we broke the feature with this check. CC @steveniemitz and @reuvenlax.
--
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
[GitHub] [beam] reuvenlax commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
reuvenlax commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-832086336
Can you explain exactly what the issue is? Setting a timer for time T with
an output timestamp of T + delta seems a bit weird to me.
The bug says that the user has no way of knowing what the timer's fire
timestamp is, but that's not true - this information is easily available.
On Tue, May 4, 2021 at 9:21 AM Kenn Knowles ***@***.***>
wrote:
> I think your change is correct. Essentially reverting the prior change,
> which broke things. On the bug it seems we found a bug in how cleanup
> timers were fired (for just Dataflow? for everyone?) and instead of fixing
> the bug we broke the feature with this check. CC @steveniemitz
> <https://github.com/steveniemitz> and @reuvenlax
> <https://github.com/reuvenlax>.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/14718#issuecomment-832070801>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFAYJVNBQDDL2VKW33XBN4TTMANJNANCNFSM44CKPPKQ>
> .
>
--
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
[GitHub] [beam] kennknowles commented on pull request #14718: [BEAM-12276] Narrow check for output timestamp of event time timers
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14718:
URL: https://github.com/apache/beam/pull/14718#issuecomment-847185865
It is OK to make a breaking change for such a new feature. It is also OK to make a breaking change when it is moving from "incorrect" to "correct".
I think I am OK with b, c, or d. The problem with (d) is of course that the window may expire downstream and be GC'd, dropping the output data. I'm not totally sure about (b) if there is some way the data can be lost.
--
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