Go stateful DoFns user side changes #22761
Conversation
|
Run Go PreCommit |
Codecov Report
@@ Coverage Diff @@
## master #22761 +/- ##
==========================================
- Coverage 74.20% 74.14% -0.07%
==========================================
Files 710 711 +1
Lines 93547 93735 +188
==========================================
+ Hits 69415 69498 +83
- Misses 22855 22957 +102
- Partials 1277 1280 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Run Go PreCommit |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @riteshghorse for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
| @@ -374,6 +390,8 @@ func New(fn reflectx.Func) (*Fn, error) { | |||
| kind = FnWindow | |||
| case t == typex.BundleFinalizationType: | |||
| kind = FnBundleFinalization | |||
| case t == state.ProviderType: | |||
There was a problem hiding this comment.
Should we have the types for state and timers defined in typex package? I was thinking this for timers as well.
There was a problem hiding this comment.
I'd probably vote we leave it as is unless there's a reason to switch - the typex package generally only exports types for structs it defines and IMO it makes sense to stay it in the state package to keep consistent with that. I don't feel super strongly though (@lostluck might have opinions too)
| WriteValueState(val Transaction) error | ||
| } | ||
|
|
||
| type PipelineState interface { |
There was a problem hiding this comment.
Doc comment. Also fine if you are planning to add it in next PRs.
There was a problem hiding this comment.
Good call, I added one.
I may end up moving this to a different package when all is said and done (most likely when I have more execution focused state stuff) since I don't really want users implementing it. It will also eventually probably need some metadata about what state type should be used (ValueState in this case). Basically, its subject to change in my next PR(s) since it's more of an internals concept.
* Go stateful DoFns user side changes * Fix static check violation * Small cleanup * Doc comments
* Go stateful DoFns user side changes * Fix static check violation * Small cleanup * Doc comments
* Go stateful DoFns user side changes * Fix static check violation * Small cleanup * Doc comments
Adds all the user facing plumbing/validation/graph construction we need to make stateful DoFns work in Go. Follows the design laid out in https://docs.google.com/document/d/1rcKa1Z6orDDFr1l8t6NA1eLl6zanQbYAEiAqk39NQUU/edit?usp=sharing
I have the full version of this (not cleaned up and not quite 100% working on the talking to the state API side) in this PR - https://github.com/damccorm/beam/pull/70/files#diff-b2583776e71d93a9777a2ababf6ae018ff116c53c905486689aced60fdf44a4e - and was able to verify that it allows us to write a pipeline with a value state object and read to/write from that state. More work is needed to get the execution piece 100%.
This is the first part of #22736 which also has next steps laid out (see my first comment in that issue).
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.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.