-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Eliminate nullness errors in KafkaIO #21783
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
Conversation
e42b329 to
629c4b4
Compare
|
R: @johnjcasey We discussed some cases. This one I did a lot more "just add an assertion" style fixes, whereas usually I like to refactor things to eliminate the possibility of any error occurring. If you can spot places where it simply doesn't make sense to have a nullable field, please comment. |
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.
I don't think these two should ever be null
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.
Done
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.
it isn't meaningful to have Kafka config without bootstrap servers, so this should throw an exception if it is null
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.
Done
5a48c83 to
2818fc8
Compare
|
And in getting tests to pass, I found that they simply didn't call |
That is very appreciated |
|
run java precommit |
2818fc8 to
fc093e9
Compare
|
run java precommit |
|
Run Python_PVR_Flink PreCommit |
1 similar comment
|
Run Python_PVR_Flink PreCommit |
|
seeing "could not find valid docker environment" across a lot of PRs, hmm |
|
Run Java PreCommit |
|
And across many PRs also org.apache.beam.sdk.io.pulsar.PulsarIOTest.testReadFromSimpleTopic: |
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
fc093e9 to
152bcde
Compare
|
Run Python_PVR_Flink PreCommit |
|
no error in the logs, and scan failed to upload :-/ |
|
Run Python_PVR_Flink PreCommit |
|
Please take another look now that tests are green. Reminder of policy: when a committer is the author, they are trusted to choose the best reviewer, including not-yet-committers. I think you are the best choice for this region of the code. And thanks for the review so far! |
|
This LGTM. Thank you for adding all these checks. |
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).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.