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/02/16 23:23:35 UTC
[GitHub] [beam] y1chi opened a new pull request #13998: [BEAM-11812] Fix ULR validate runner test failure caused by merging I…
y1chi opened a new pull request #13998:
URL: https://github.com/apache/beam/pull/13998
…nvalidWindow
**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 | 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 a change in pull request #13998: [BEAM-11812] Fix ULR validate runner test failure caused by merging I…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #13998:
URL: https://github.com/apache/beam/pull/13998#discussion_r577326177
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/InvalidWindows.java
##########
@@ -87,4 +87,9 @@ public void verifyCompatibility(WindowFn<?, ?> other) throws IncompatibleWindowE
public Instant getOutputTime(Instant inputTimestamp, W window) {
return inputTimestamp;
}
+
+ @Override
+ public boolean isNonMerging() {
Review comment:
It might be even better to change to `extends NonMergingWindowFn`.
I also think: wow! we chose the wrong default value for 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
[GitHub] [beam] kennknowles commented on pull request #13998: [BEAM-11812] Fix ULR validate runner test failure caused by merging I…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #13998:
URL: https://github.com/apache/beam/pull/13998#issuecomment-780313629
See #14001 which might be related. CC @robertwb here too.
In portability, I wonder if `InvalidWindows` is obsolete. The only reason for it was to crash in this case:
1. User sets keys and chooses session windowing
3. User groups/combines by session
4. User maps the outputs to some new keys
5. User groups again
In the original model, the second grouping would do merging again, and probably be something random that they did not intend. Or anyhow that is how I remember the reasoning.
Now I think `MergeStatus.ALREADY_MERGED` would allow better behavior. In the second group by key the windows would be treated as non-merging.
Was there any discussion of `MergeStatus` that I may have missed?
----------------------------------------------------------------
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] robertwb commented on a change in pull request #13998: [BEAM-11812] Fix ULR validate runner failure caused by merging InvalidWindow
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #13998:
URL: https://github.com/apache/beam/pull/13998#discussion_r578847151
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/InvalidWindows.java
##########
@@ -87,4 +87,9 @@ public void verifyCompatibility(WindowFn<?, ?> other) throws IncompatibleWindowE
public Instant getOutputTime(Instant inputTimestamp, W window) {
return inputTimestamp;
}
+
+ @Override
+ public boolean isNonMerging() {
Review comment:
+1
----------------------------------------------------------------
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] TheNeuralBit commented on pull request #13998: [BEAM-11812] Fix ULR validate runner failure caused by merging InvalidWindow
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13998:
URL: https://github.com/apache/beam/pull/13998#issuecomment-791834678
This is obsolete now that #14059 is in.
----------------------------------------------------------------
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] y1chi commented on pull request #13998: [BEAM-11812] Fix ULR validate runner test failure caused by merging I…
Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #13998:
URL: https://github.com/apache/beam/pull/13998#issuecomment-780200227
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] TheNeuralBit closed pull request #13998: [BEAM-11812] Fix ULR validate runner failure caused by merging InvalidWindow
Posted by GitBox <gi...@apache.org>.
TheNeuralBit closed pull request #13998:
URL: https://github.com/apache/beam/pull/13998
----------------------------------------------------------------
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] y1chi commented on pull request #13998: [BEAM-11812] Fix ULR validate runner test failure caused by merging I…
Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #13998:
URL: https://github.com/apache/beam/pull/13998#issuecomment-780199881
R: @TheNeuralBit @robertwb
----------------------------------------------------------------
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] TheNeuralBit commented on pull request #13998: [BEAM-11812] Fix ULR validate runner test failure caused by merging I…
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13998:
URL: https://github.com/apache/beam/pull/13998#issuecomment-780238812
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 #13998: [BEAM-11812] Fix ULR validate runner test failure caused by merging I…
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #13998:
URL: https://github.com/apache/beam/pull/13998#issuecomment-780314218
To be clear: we should fix the CI signal, but I think best to sickbay the test until this is sorted out. Probably the right thing for `InvalidWindows` to do is to delegate `isNonMerging` to the original windowFn. But at runtime the right thing for it to do is to crash! So we don't want to fix the crash, but figure out why it was being called. The tests exists specifically to catch the failure that has occurred.
----------------------------------------------------------------
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