You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@beam.apache.org by GitBox <gi...@apache.org> on 2020/02/27 18:04:14 UTC
[GitHub] [beam] acrites opened a new pull request #10988: [BEAM-9382] Clean
up of TestStreamTranscriptTests.
acrites opened a new pull request #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988
Removes check for final field in PaneInfo since it is not reported properly by direct runner. Updates index for session panes and changes test with Count(2) early trigger to only inject two elements since behavior with 3 is non-deterministic.
R: @robertwb
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-598262109
Run Python 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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-598366163
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-593667609
Drive by comment:
Shouldn't we disable the test for the direct runner if it isn't supported and file a bug instead of editing what the test does?
Alternatively, could we fix the direct runner?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] kennknowles commented on issue #10988: [BEAM-9382] Clean up
of TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
kennknowles commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-597288570
Yea, that's 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
With regards,
Apache Git Services
[GitHub] [beam] kennknowles commented on issue #10988: [BEAM-9382] Clean up
of TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
kennknowles commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-598878883
Yea I don't think anyone is happy about it. But it has always been that way and also is the only way we meet the spec of panes being (EARLY* ON_TIME? LATE*)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-597194043
Run Python 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
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-597287538
It looks like TriggerDriverTranscriptTest.test_early_late_sessions is failing (since there's no way to sickbay that test). Am I understanding the semantics of PaneInfo index correctly? The first firing of each window should have index 0 even if that window was the result of merging another window, 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
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-593753721
Along the lines of what Kenn is saying, I had originally thought that these tests weren't really testing whether or not PaneInfo.final gets set correctly in the various triggering strategies, but more so that it was outputting the correct elements. Maybe we want to test all aspects in a single test though.
I'm fine with sickbaying the tests for Python direct runner if you're ok with having a large number of tests inactive until we getting around to fixing direct runner. Maybe that'll motivate us to get it fixed faster...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-598997790
Apparently the failing test in the PreCommit has been "Failing for the past 11,721 builds (Since #0 )"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-597190980
Actually, that last example doesn't fail like I would expect. So I guess the Python direct runner sets final = true sometimes...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] robertwb merged pull request #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
robertwb merged pull request #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] robertwb commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
robertwb commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-595930667
The Python direct runner does support PaneInfo, but it doesn't (yet) support allowed lateness, so the "final" bit is wrong. Yes, we should fix this, though hopefully on the FnApiRunner once it supports streaming rather than invest too much into the old direct runner (unless it's cheap to do).
I would rather disable the "final" bit check for these tests (and add a new, disabled test that checks this bit) than disable these tests entirely for the direct runner. So as is, this change LGTM.
We could also consider expanding the API to indicate certain features are broken on certain runners, rather than entire tests, which would allow us to fully specify the expected results and still produce partial validation on incomplete/in-progress runners.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-596263133
RE fixing the Python direct runner: right now we just use the return value
from on_fire for each trigger to set the final bit. This doesn't take into
account allowed lateness. So we'd probably need to update all of the
triggers.
Maybe just a single test for now that is disabled and has the final bit
check? Or did you want to duplicate all the tests? I'm thinking something
like this one:
name: fixed_allowed_lateness_final_set
broken_on:
- SwitchingDirectRunner # Doesn't set final field properly with late
data.
window_fn: FixedWindows(10)
trigger_fn: AfterWatermark(early=AfterCount(2), late=AfterCount(1))
timestamp_combiner: OUTPUT_AT_EOW
allowed_lateness: 10
transcript:
- input: [1, 2]
- expect:
- {window: [0, 9], values: [1, 2], timestamp: 9, final: false}
- input: [3]
- watermark: 100
- expect:
- {window: [0, 9], values: [1, 2, 3], timestamp: 9, final: true}
On Fri, Mar 6, 2020 at 11:45 AM Robert Bradshaw <no...@github.com>
wrote:
> The Python direct runner does support PaneInfo, but it doesn't (yet)
> support allowed lateness, so the "final" bit is wrong. Yes, we should fix
> this, though hopefully on the FnApiRunner once it supports streaming rather
> than invest too much into the old direct runner (unless it's cheap to do).
>
> I would rather disable the "final" bit check for these tests (and add a
> new, disabled test that checks this bit) than disable these tests entirely
> for the direct runner. So as is, this change LGTM.
>
> We could also consider expanding the API to indicate certain features are
> broken on certain runners, rather than entire tests, which would allow us
> to fully specify the expected results and still produce partial validation
> on incomplete/in-progress runners.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/10988?email_source=notifications&email_token=ACOKCJXEORU3BA5EY2IWAFTRGFHD7A5CNFSM4K5AFZM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOCS4KY#issuecomment-595930667>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACOKCJR3YOU4QZIDUJOT7DLRGFHD7ANCNFSM4K5AFZMQ>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] kennknowles commented on issue #10988: [BEAM-9382] Clean up
of TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
kennknowles commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-593740435
Since we are doing drive by comments, perhaps a clear comment describing exactly what the test is trying to verify would allow us to be sure to have all the necessary verifications but no extraneous info.
Also I assume you mean the Python direct runner (each language has a direct runner for that language) since I think the Java direct runner does support pane info.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] robertwb commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
robertwb commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-598853844
I'm not convinced that reseting the indices to 0 on merging windows is the desired semantics, but that discussion is probably out of scope for this PR. Other than that, LGTM.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [beam] acrites commented on issue #10988: [BEAM-9382] Clean up of
TestStreamTranscriptTests.
Posted by GitBox <gi...@apache.org>.
acrites commented on issue #10988: [BEAM-9382] Clean up of TestStreamTranscriptTests.
URL: https://github.com/apache/beam/pull/10988#issuecomment-597797005
Run Python 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
With regards,
Apache Git Services