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/03/18 04:27:58 UTC
[GitHub] [beam] kennknowles opened a new pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
kennknowles opened a new pull request #14268:
URL: https://github.com/apache/beam/pull/14268
See [dev@ thread](https://lists.apache.org/thread.html/r99d0efa70f83815e17a1dbfb94effd16b38176046ac8ffbfe371c895%40%3Cdev.beam.apache.org%3E) where consensus is that the existence of this method is a _bug_ and it should be eliminated.
See next [user@ thread](https://lists.apache.org/thread.html/r7220ad2732e3d93e23e1f8af8051c5f6f6d72efdf556a467b9afb9fc%40%3Cuser.beam.apache.org%3E) where no one responded. Perhaps because it is complex and esoteric.
Important points to consider about this change, worth repeating in the PR:
- the method is annotated "experimental" but it has been around since the very early days of Beam so we should ignore that
- it was added as response to a _real user problem_: sliding windows + `EARLIEST` timestamp combiner delays downstream aggregations a lot
- but when that problem occurred, timestamp combiner `EARLIEST` was the default; with the change to how lateness is defined we changed the default to `END_OF_WINDOW`
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] 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.
- [x] Update `CHANGES.md` with noteworthy changes.
- [x] 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 | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam
.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.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.
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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-819118576
Run Java_Examples_Dataflow 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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803134997
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803068888
run flink validatesrunnner
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-813708328
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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-819100724
Everything was green except for what looked like a flake in Java PreCommit unable to bind a local port. I just rebased with master.
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-814402634
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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803069630
run dataflow 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
[GitHub] [beam] boyuanzz commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-812215546
A high level question regarding to
> it was added as response to a real user problem: sliding windows + EARLIEST timestamp combiner delays downstream aggregations a lot
but when that problem occurred, timestamp combiner EARLIEST was the default; with the change to how lateness is defined we changed the default to END_OF_WINDOW
With this PR, what will happen to the real user problem mentioned 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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803068753
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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-811433042
Synced with master since it has been a while. Will re-run tests now that more postcommits are green on master.
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-813504955
> > A high level question regarding to
> > > it was added as response to a real user problem: sliding windows + EARLIEST timestamp combiner delays downstream aggregations a lot
> > > but when that problem occurred, timestamp combiner EARLIEST was the default; with the change to how lateness is defined we changed the default to END_OF_WINDOW
> >
> >
> > With this PR, what will happen to the real user problem mentioned above?
>
> Short answer: the real user problem will be back but not as bad.
>
> Longer answer: EARLIEST used to be the default when `getOutputTime` was introduced to solve the problem. Now the default is END_OF_WINDOW so the problem is not as bad. To use sliding windows (or other overlapping windows) with EARLIEST, the user has some choices:
>
> * Just be OK with the delay of downstream GBK (already true for Python)
> * Set up a non-default trigger (this will free watermark holds and allow progress downstream)
> * Manually move element timestamps forward with a `ParDo` before GBK, achieving identical behavior
>
> I think further discussion should continue on the dev@ thread instead of the PR though.
Thanks for the explanation. If it's something more like a common issue, we should document this in the release note or somewhere else.
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803203666
Streaming VR suite appears to be perma-red actually, so this is not a useful signal.
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-811436590
Run Flink 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
[GitHub] [beam] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-811438105
Run ULR Loopback 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
[GitHub] [beam] kennknowles merged pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles merged pull request #14268:
URL: https://github.com/apache/beam/pull/14268
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-813701139
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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-801617437
I think it ended up being ambiguous whether we should temporarily introduce an `EARLIEST_SHIFTED` timestamp combiner. I have not included it in this change, based on zero replies on `user@` (admittedly a very low subscription list).
----------------------------------------------------------------
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803084557
Run Flink 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
[GitHub] [beam] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803070012
run dataflow streaming 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
[GitHub] [beam] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-813487929
> A high level question regarding to
>
> > it was added as response to a real user problem: sliding windows + EARLIEST timestamp combiner delays downstream aggregations a lot
> > but when that problem occurred, timestamp combiner EARLIEST was the default; with the change to how lateness is defined we changed the default to END_OF_WINDOW
>
> With this PR, what will happen to the real user problem mentioned above?
Short answer: the real user problem will be back but not as bad.
Longer answer: EARLIEST used to be the default when `getOutputTime` was introduced to solve the problem. Now the default is END_OF_WINDOW so the problem is not as bad. To use sliding windows (or other overlapping windows) with EARLIEST, the user has some choices:
- Just be OK with the delay of downstream GBK (already true for Python)
- Set up a non-default trigger (this will free watermark holds and allow progress downstream)
- Manually move element timestamps forward with a `ParDo` before GBK, achieving identical behavior
I think further discussion should continue on the dev@ thread instead of the PR though.
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-804441019
The actual failure was unrelated:
```
12:42:57 > Task :runners:google-cloud-dataflow-java:validatesRunnerLegacyWorkerTestStreaming
14:26:37
14:26:37 org.apache.beam.sdk.transforms.ParDoTest$StateTests > testMapState FAILED
14:26:37 java.lang.RuntimeException at ParDoTest.java:2408
14:48:08
14:48:08 org.apache.beam.sdk.transforms.ParDoTest$StateTests > testSetState FAILED
14:48:08 java.lang.RuntimeException at ParDoTest.java:2352
```
This is fixed by #14302
--
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 #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-813504296
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] kennknowles commented on pull request #14268: [BEAM-12011] Eliminate WindowFn.getOutputTime method
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #14268:
URL: https://github.com/apache/beam/pull/14268#issuecomment-803068952
run spark 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