-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-7029] Add KafkaIO.Write as an external transform #8322
Conversation
Run Python PreCommit |
Planning to merge this later today if there are no further comments. |
We should allow the user to specify the environment for the external transform. Would like to be able to use EMBEDDED when using the Flink runner. |
I already discovered this before opening this PR and created https://jira.apache.org/jira/browse/BEAM-7084. I think this can be solved independently of this PR. |
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.
Added a couple of minor notes.
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java
Outdated
Show resolved
Hide resolved
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java
Outdated
Show resolved
Hide resolved
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 Max. LGTM other than few minor comments.
self.assertTrue('No resolvable bootstrap urls given in bootstrap.servers' | ||
in str(ctx.exception), | ||
'Expected to fail due to invalid bootstrap.servers, but ' | ||
'failed due to:\n%s' % str(ctx.exception)) | ||
|
||
# We just test the expansion but do not execute. |
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.
FYI: @ihji is looking into adding a validates runner test suite for cross-language transforms that includes Kafka.
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.
Yes, I saw the PR (#8397). That will be great to extend our test coverage here.
This adds KafkaIO.Write as an external transform and includes a Python wrapper (WriteToKafka) for convenience.
Thanks for all the comments! |
This adds KafkaIO.Write as an external transform and includes a Python
wrapper (WriteToKafka) for convenience.
CC @chamikaramj @ihji @robertwb @tweise
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.