Add outputWindowedValue capability to Java SDK by kennknowles · Pull Request #29616 · apache/beam

kennknowles · 2023-12-05T15:54:21Z

This adds the ability (which we don't expect to use much, and not really at all from users) to output a full windowed value. This allows a proper implementation of Reshuffle in the SDK and could be useful in some other testing scenarios.

Pulling this off #28853 for simpler review while working out final issues there.

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

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
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.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

kennknowles · 2023-12-05T16:03:16Z

R: @robertwb

github-actions · 2023-12-05T16:04:24Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

kennknowles · 2023-12-05T19:56:30Z

R: @johnjcasey since you also already took a look earlier

johnjcasey

LGTM, just some small comments.

I'm far from expert on the FnApiDoFnRunner, so you may want another set of eyes there

johnjcasey · 2023-12-05T20:27:54Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Create.java

looks like some typos in the docstring

Ah yes. Copy/paste/forget of course :-)

Thanks!

johnjcasey · 2023-12-05T20:28:30Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Create.java

johnjcasey · 2023-12-06T15:15:38Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Create.java

Unrelated to this pr, but this coder lookup pattern might be useful to extract to a utility

Yea, I need to see about this. I am actually the original author of the "infer coder from raw values" logic back in the day. It has been improved and also had the schema stuff added so I don't 100% know how best to generalize.

Also I noticed in building this that window types are not in the registry, because WindowFn already tells the pipeline what coder to use. Probably not a big deal since it never mattered until this PR.

johnjcasey · 2023-12-06T15:18:58Z

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java

Is validity jus that the timestamp is in the window?

TBH I just copy/pasted the comment. Filed #29637 and linked it to each of the TODOs.

kennknowles

Thanks for the excellent eye for detail.

kennknowles · 2023-12-06T16:39:17Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Create.java

Ah yes. Copy/paste/forget of course :-)

Thanks!

kennknowles · 2023-12-06T16:39:40Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Create.java

kennknowles · 2023-12-06T16:41:04Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Create.java

Yea, I need to see about this. I am actually the original author of the "infer coder from raw values" logic back in the day. It has been improved and also had the schema stuff added so I don't 100% know how best to generalize.

Also I noticed in building this that window types are not in the registry, because WindowFn already tells the pipeline what coder to use. Probably not a big deal since it never mattered until this PR.

kennknowles · 2023-12-06T16:44:19Z

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java

TBH I just copy/pasted the comment. Filed #29637 and linked it to each of the TODOs.

kennknowles · 2023-12-13T15:41:32Z

Rebased to get test fixes but actually if we read configuration from master then it was not needed. Now tests will time out sooner and still upload test results I think. We were being blocked by #28957 and there is no known issue with this PR. But it is a big change and I want to merge on green, despite this causing weeks of delay :-(

Abacn · 2024-02-02T22:30:48Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/DoFn.java

+     * <p><i>Note:</i> A splittable {@link DoFn} is not allowed to output from {@link StartBundle}
+     * or {@link FinishBundle} methods.
+     */
+    public abstract void outputWindowedValue(


This new abstract method introduces a breaking change in Beam 2.54.0 for classes implemented OutputReceiver

Abacn · 2024-02-02T22:39:10Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/DoFn.java


    void outputWithTimestamp(T output, Instant timestamp);
+
+    void outputWindowedValue(


Actually it is this one.

github-actions bot added java io extensions runners sql gcp kafka core spanner labels Dec 5, 2023

kennknowles force-pushed the outputWindowedValue branch from 7ac9ce9 to 2c5e7fd Compare December 5, 2023 16:02

kennknowles force-pushed the outputWindowedValue branch 5 times, most recently from c25575f to 087382e Compare December 5, 2023 18:40

johnjcasey approved these changes Dec 6, 2023

View reviewed changes

kennknowles force-pushed the outputWindowedValue branch from 087382e to 41690b5 Compare December 6, 2023 16:47

kennknowles commented Dec 6, 2023

View reviewed changes

kennknowles force-pushed the outputWindowedValue branch 3 times, most recently from 39023cd to a75e53a Compare December 8, 2023 13:21

Add outputWindowedValue capability to Java SDK

db0bb44

kennknowles force-pushed the outputWindowedValue branch from a75e53a to db0bb44 Compare December 13, 2023 15:37

kennknowles merged commit ef0ee76 into apache:master Dec 13, 2023

kennknowles deleted the outputWindowedValue branch December 13, 2023 20:56

Abacn reviewed Feb 2, 2024

View reviewed changes

Abacn mentioned this pull request Feb 2, 2024

[Bug]: Newly introduced OutputReceiver.outputWindowedValue breaking downstream user transforms #30203

Closed

16 tasks


		void outputWithTimestamp(T output, Instant timestamp);

		void outputWindowedValue(

Conversation

kennknowles commented Dec 5, 2023

GitHub Actions Tests Status (on master branch)

Uh oh!

kennknowles commented Dec 5, 2023

Uh oh!

github-actions bot commented Dec 5, 2023

Uh oh!

kennknowles commented Dec 5, 2023

Uh oh!

johnjcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennknowles commented Dec 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants