Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-8575] Add a Python test to test windowing in DoFn finish_bundle() #10145

Merged
merged 2 commits into from Nov 20, 2019

Conversation

liumomo315
Copy link
Contributor

This test is the Python parity for the Java ParDo test testWindowingInStartAndFinishBundle.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) 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.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@liumomo315
Copy link
Contributor Author

R: @y1chi

PTAL Yichi! Thanks:)

@y1chi
Copy link
Contributor

y1chi commented Nov 19, 2019

LGTM, I wonder would it be better to move the timestamp assignment to beam.Create() and combine the map function into the MyDoFn.

@liumomo315
Copy link
Contributor Author

liumomo315 commented Nov 19, 2019

Thanks for the quick review!

move the timestamp assignment to beam.Create()

This can be done, but does not make much difference, since we still want both the process() and finish_bundle() to do something in this test, and see the reason below.

and combine the map function into the ParDo.

I think it's clearer to separate the Map from the test DoFn. The purpose of this test is to verify that after a DoFn with finish_bundle() implemented, it will produce results both from process() and finish_bundle(). More specifically, it wants to make sure that when windowing is involved, the output will be correct after the DoFn. The last Map is simply to print out all outputs from the test DoFn. Thoughts?

@y1chi
Copy link
Contributor

y1chi commented Nov 19, 2019

@tvalentyn could you help to merge?

@liumomo315
Copy link
Contributor Author

Thanks Yichi for the review:)

@tvalentyn tvalentyn merged commit d1b513d into apache:master Nov 20, 2019
@tvalentyn
Copy link
Contributor

tvalentyn commented Nov 20, 2019

Thanks, @liumomo315 and @y1chi ! Somehow I missed the last comment, merged now!

@tvalentyn
Copy link
Contributor

@liumomo315 Please run ValidatesRunner tests when marking new tests as "VR". The command to trigger the tests can be found in .test-infra/jenkins/README file, the link is available in the PR template. To run those tests you can just comment on the PR: Run Dataflow Python ValidatesRunner

@liumomo315
Copy link
Contributor Author

Thanks for the hints, Valentyn! Will do that for my new PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants