[BEAM-14470] Use lifecycle method names directly.#17790
[BEAM-14470] Use lifecycle method names directly.#17790lostluck merged 3 commits intoapache:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17790 +/- ##
==========================================
+ Coverage 74.00% 74.08% +0.07%
==========================================
Files 695 697 +2
Lines 91898 91927 +29
==========================================
+ Hits 68013 68103 +90
+ Misses 22639 22580 -59
+ Partials 1246 1244 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
jrmccluskey
left a comment
jrmccluskey
left a comment
There was a problem hiding this comment.
LGTM, should get a look rom @damccorm as well since they should have more context
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
LGTM - could you please file (and link to) a Jira to revert once it is no longer breaking?
|
As discussed offline, we decided to completely remove the method restriction entirely. While this may make things a little harder for novices, it will make things harder for novices with typos in the optional methods. These should be detectable by users via testing however. Unfortunately there's no good way of validating this on our end short of doing some sort of typo/proximity algorithm, which is likely more trouble than better documentation and clearer examples, hence the initial strict approach. |
|
@damccorm PTAL |
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
For posterity, we decided offline to remove verifyValidNames because the current change effectively circumvents that check in some cases and it doesn't make sense to only enforce it some of the time and/or leave behind an unenforced restriction. We're not planning on ever enforcing this restriction going forward (at least not without a major version bump).
| } | ||
| } | ||
|
|
||
| func TestNewFn_SplittableDoFn(t *testing.T) { |
There was a problem hiding this comment.
It would probably be good to add an extra similar test case for combineFns as well.
There was a problem hiding this comment.
Ah, I knew I missed something. Thank you!
We also have a vestigial "CompactFn" It's intended to allow CombineFns to do compression of the accumulators, but it seems it's not documented or called anywhere. Out of scope to delete it in this PR, but it could be worth implementing for user use eventually.
3 participants
Use lifecycle method names directly, rather than excluding all exported methods on DoFns.
Breaks certain usecases with extreme number (10+) of emitter parameters, when the method doesn't have a generic representation. The static code generator works in those cases, but it isn't recommended.
The ideal work around would be to have a tagged API instead of the positional one, but that's significantly more work.
Using lifecycle names directly neutered the previous verification check anyway, so we've opted to remove that entirely. Additional tests were also added to ensure the fallthrough behavior occurs.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.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.