-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-4420] Add KafkaIO Integration Tests #8986
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
This is useful for ad hoc configuration amendments etc. and helpful for everyday development.
|
Checkstyle not happy, you should use the vendored version of guava. Can you please also squash the commits into one with the proper title. Thanks. |
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.
Thanks! Additionally to what Ismael said, I added several review comments
sdks/java/io/kafka/src/test/java/org/apache/beam/sdk/io/kafka/KafkaIOIT.java
Outdated
Show resolved
Hide resolved
sdks/java/io/kafka/src/test/java/org/apache/beam/sdk/io/kafka/KafkaIOIT.java
Outdated
Show resolved
Hide resolved
sdks/java/io/kafka/src/test/java/org/apache/beam/sdk/io/kafka/KafkaIOIT.java
Show resolved
Hide resolved
sdks/java/io/kafka/src/test/java/org/apache/beam/sdk/io/kafka/KafkaIOIT.java
Outdated
Show resolved
Hide resolved
|
@iemejia @aromanenko-dev thanks for the review. I provided fixes. @iemejia I didn't squash the commits on purpose - those are two atomic, independent changes. Having them as separate commits shows better why exactly something changed with proper commit titles and descriptions (see the description for the Kubernetes change). Squash will obfuscate this and will result with multiple unrelated changes in one commit. Other than that it will be easier to revert parts if needed. Suppose I was to revert only the Kubernetes change. Should I be forced to revert the whole test too? |
aromanenko-dev
left a 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.
LGTM
Agree to keep two independent commits
|
Thanks! |
This pr adds basic IOIT that can be run for 1000 records (there's only hashcode for 1000 records currently). To make the code review easier and because I'm not a Kafka expert, I took a baby steps approach here - after this PR is approved consequent PRs will provide:
I ran this test on existing kubernetes kafka cluster that is there in .test-infra folder (with success).
@aromanenko-dev @iemejia could you take a look?
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.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.