[Go SDK] Add timer coder support #23222
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23222 +/- ##
==========================================
- Coverage 73.58% 73.54% -0.04%
==========================================
Files 716 716
Lines 95316 95408 +92
==========================================
+ Hits 70140 70172 +32
- Misses 23880 23925 +45
- Partials 1296 1311 +15
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 |
|
R: @lostluck |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
| @@ -1208,3 +1213,91 @@ func DecodeWindowedValueHeader(dec WindowDecoder, r io.Reader) ([]typex.Window, | |||
|
|
|||
| return ws, t, pn, nil | |||
| } | |||
|
|
|||
| // EncodeTimer encodes a typex.TimerMap into a byte stream. | |||
| func EncodeTimer(elm ElementEncoder, tm typex.TimerMap, w io.Writer) error { | |||
There was a problem hiding this comment.
Does this need to be exported?
Can we move it, or parts of it into the graph/coder package along with the other concrete coder implementations (I suspect it will probably live here with the window header for the same reasons though...)
There was a problem hiding this comment.
It doesn't need to. I had both of these encode and decode in graph/coder/ but it creates weird import cycle. To separate some parts, I think Encoding will work fine but we need to pass in exec.ElementDecoder and exec.WindowDecoder to Decode which creates weird import cycle.
|
To better check the new coder, you'll also want to enable it in the standard_coders.yaml test file we have: That will run/fail on timer test cases from here: |
|
retest this please |
| @@ -36,6 +36,7 @@ var ( | |||
|
|
|||
| EventTimeType = reflect.TypeOf((*EventTime)(nil)).Elem() | |||
| WindowType = reflect.TypeOf((*Window)(nil)).Elem() | |||
| TimersType = reflect.TypeOf((*Timers)(nil)).Elem() | |||
There was a problem hiding this comment.
This seems like the only place that Timers is used (at least in this PR). Why can't this be TimersMap?
There was a problem hiding this comment.
TimerMap is just a placeholder for timer details but Timers is the actual type used in standard timer coder.
|
made the changes, PTAL |
Adds timer coder and decoder with test
Part of #22737
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.