apache
Update Beam Protobuf Schema (Java)
Fix #35081.
Refer to https://s.apache.org/beam-protobuf.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] 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, commentfixes #<ISSUE NUMBER>instead. - [ ] Update
CHANGES.mdwith noteworthy changes. - [x] 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.
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Assigning reviewers:
R: @m-trieu for label java. R: @damccorm for label build.
Note: If you would like to opt out of this review, comment assign to next reviewer.
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
The current fix for ProtoMessageSchema uses hasField(FieldDescriptor), but it's not efficient.
I've just realized that a scalar field with "presence" has a has<FieldName> method. I'll use ByteBuddy to make more efficient code.
I set up at least the logicaltypes folder to have the analysis enabled in #35217. I did not want to create extra scope for your change, but I also think that having it fully enabled will make things turn out better, considering the nature of this change.
I haven't finish a full review but I have two big picture comments:
- The amount of
@NonNullannotations is suspicious, since this is the default. You should very rarely need to use it. There are also many places where you apply@Nullableto a type variable, which is usually rare, but I think might be fine in some of these cases.- It feels like the many subclasses really might be simple as just functions / a
switchstatement.
Thank you so much for the review! I have refactored the ProtoBeamConverter class.
I've cherry-picked the commit at https://github.com/apache/beam/pull/35217. Thanks!
@kennknowles could you please proceed with the review when you have time? Thank you.
I haven't gotten to make another pass yet. I did just look at the tests and there is a prism failure which is pretty rare. I restarted the test job anyhow but you might take a look at the gradle scan at https://develocity.apache.org/s/o35wszaauzfvy/tests/task/:runners:prism:java:test/details/org.apache.beam.runners.prism.PrismRunnerTest/windowing?top-execution=1 just to make sure it really was a flake
I haven't gotten to make another pass yet. I did just look at the tests and there is a prism failure which is pretty rare. I restarted the test job anyhow but you might take a look at the gradle scan at https://develocity.apache.org/s/o35wszaauzfvy/tests/task/:runners:prism:java:test/details/org.apache.beam.runners.prism.PrismRunnerTest/windowing?top-execution=1 just to make sure it really was a flake
The test passed locally, so I triggered a rerun and it's now green.
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers
I think that the many cases of @NonNull and @Nullable applied to type variables indicates a problem. Each one really needs a comment justifying it, because it is unusual and actually a somewhat inflexible situation. Probably you can remove most of them. And some places where you have a type variable and you need to constrain what types it can be instantiated with, you would use extends @NonNull Object (the default if you do not annotate is extends @Nullable Object which allows the type variable to be instantiated with any type)
I've removed @NonNull and @Nullable from generic types in the ProtoBeamConverter class.
I realized I had significantly misunderstood their proper usage. Nullability annotations on generics are only for specific use cases with strong reasoning (apparently a container-like class?).
Hello @kennknowles , are you able to review this PR when again? Thank you.
Confirmed that both test failures look like flakes. I'm re-running them.
Thanks for the fix ! Friendly ask : would you do a patch release of beam that includes this fix ?
Thanks for the fix ! Friendly ask : would you do a patch release of beam that includes this fix ?
We usually do not do a patch release. If you need this now, you can use the SNAPSHOT version: https://github.com/apache/beam/blob/master/contributor-docs/code-change-guide.md#run-your-pipeline-with-modified-beam-code
Probably caused https://github.com/apache/beam/actions/workflows/beam_PreCommit_Yaml_Xlang_Direct.yml?query=event%3Aschedule failing
update: confirmed: https://github.com/apache/beam/actions/runs/18021430173
@Abacn it still failed after the revert: https://github.com/apache/beam/actions/runs/18023411365. I doubt if this change really caused the test failing.