Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Eliminate nullness errors in KafkaIO #21783

Merged
merged 2 commits into from
Jun 17, 2022
Merged

Eliminate nullness errors in KafkaIO #21783

merged 2 commits into from
Jun 17, 2022

Conversation

kennknowles
Copy link
Member

@kennknowles kennknowles commented Jun 9, 2022 •
edited
Loading

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:

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)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@kennknowles kennknowles force-pushed the kafka-nullness branch 2 times, most recently from e42b329 to 629c4b4 Compare June 9, 2022 23:00
@kennknowles
Copy link
Member Author

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.

johnjcasey
johnjcasey reviewed Jun 10, 2022
View reviewed changes
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/ConsumerSpEL.java Outdated
@@ -117,7 +118,7 @@ public static void evaluateSeek2End(Consumer<?, ?> consumer, TopicPartition topi
}

public static void evaluateAssign(
Consumer<?, ?> consumer, Collection<TopicPartition> topicPartitions) {
@Nullable Consumer<?, ?> consumer, @Nullable Collection<TopicPartition> topicPartitions) {
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaExactlyOnceSink.java Outdated
Preconditions.checkArgumentNotNull(spec.getConsumerFactoryFn());

Map<String, Object> consumerConfig = new HashMap<>();
Object bootStrapServersConfig =
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kennknowles kennknowles force-pushed the kafka-nullness branch 2 times, most recently from 5a48c83 to 2818fc8 Compare June 10, 2022 18:04
@kennknowles
Copy link
Member Author

And in getting tests to pass, I found that they simply didn't call setup(), so now the tests are more accurate to real usage.

@github-actions github-actions bot added the build label Jun 10, 2022
@johnjcasey
Copy link
Contributor

And in getting tests to pass, I found that they simply didn't call setup(), so now the tests are more accurate to real usage.

That is very appreciated

@kennknowles kennknowles reopened this Jun 13, 2022
@kennknowles
Copy link
Member Author

run java precommit

@kennknowles
Copy link
Member Author

run java precommit

@kennknowles
Copy link
Member Author

Run Python_PVR_Flink PreCommit

1 similar comment
@kennknowles
Copy link
Member Author

Run Python_PVR_Flink PreCommit

@kennknowles
Copy link
Member Author

seeing "could not find valid docker environment" across a lot of PRs, hmm

@kennknowles
Copy link
Member Author

Run Java PreCommit

@kennknowles
Copy link
Member Author

And across many PRs also org.apache.beam.sdk.io.pulsar.PulsarIOTest.testReadFromSimpleTopic: Trying to claim offset 1655305408194 while last attempted was 1655305409570

@kennknowles
Copy link
Member Author

Run Java PreCommit

1 similar comment
@kennknowles
Copy link
Member Author

Run Java PreCommit

@kennknowles
Copy link
Member Author

Run Python_PVR_Flink PreCommit

@kennknowles
Copy link
Member Author

no error in the logs, and scan failed to upload :-/

@kennknowles
Copy link
Member Author

Run Python_PVR_Flink PreCommit

@kennknowles
Copy link
Member Author

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!

@johnjcasey
Copy link
Contributor

This LGTM. Thank you for adding all these checks.

@kennknowles kennknowles merged commit 3a6100d into apache:master Jun 17, 2022
@kennknowles kennknowles deleted the kafka-nullness branch June 17, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnjcasey johnjcasey johnjcasey left review comments

Assignees
No one assigned
Labels
build io java kafka
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kennknowles @johnjcasey