Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Go SDK] Timers with new datalayer #26101
[Go SDK] Timers with new datalayer #26101
Changes from 16 commits
6cdbcd0
f0e9ba6
e5ec39b
15386c6
4fe6307
49ead2b
efe50cf
77b1438
3e0e4de
49407e0
4191839
dd0c4e6
95d641f
6ce7814
9762682
5b012f5
64d4759
afff87e
601dd58
6ea3969
06bad75
1c9e57b
161f5a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example only includes the key. If I want to access the value in
OnTimer
, will I have to pass it through the state API? I see that the design says a value should be mapped, but this says the interaction is through state API. I have not been able to add an input value, getting some variant ofSimilarly I have not been able to add any emitter, getting similar errors as per above.
Another issue I had was that it failed to validate when the
beam.EventTime
was placed after thetimers.Provider
. It says then that it needs to be before the main input. Moving it into the same position as here seems to work.Finally I was unable to add timers to a GBK DoFn, having it complain about needing to use KV with timers, but I need to investigate further on this one. Maybe it is not supposed to be possible to use timers in such cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your interest!
The design doc is a little out of date, and was authored with a less than complete understanding of timers. The Beam Model doc is correct and any values would need to be carried over via the state API.
The FnAPI simply doesn't have a provision for the additional data.
Timers and State are implicitly per key, but timers can have an additional "tag" associated with them for an additional uniqueness factor.
We definitely do need to have emitters working (which would always match the ProcessElement emitters exactly), and it's probably simply just not implemented in this WIP PR. Adding new "lifecycle" methods isn't simple.
I believe that GBKs should be able to have a timer callback and associated state it's largely just a validation thing on the SDK that needs updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing it out. I got diverted to other part of the work. I've added support for emitters now. Feel free to test your pipelines