-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(ingest/kafka-connect): add support for Confluent S3 Sink Connector #8298
feat(ingest/kafka-connect): add support for Confluent S3 Sink Connector #8298
Conversation
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.
Overall looks good.
Left some comments around topics config and tests.
metadata-ingestion/src/datahub/ingestion/source/kafka_connect.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/kafka_connect.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/tests/integration/kafka-connect/test_kafka_connect_s3sink.py
Outdated
Show resolved
Hide resolved
0fe2751
to
c5a9644
Compare
metadata-ingestion/tests/integration/kafka-connect/kafka_connect_s3sink_to_file.yml
Show resolved
Hide resolved
@mayurinehate anything else needed to merge this? |
Hey @tusharm, Thank you for patience. Once CI passes, it should be good to merge from my side. |
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.
Approving based on Mayuri's recommendation
hey @anshbansal, I see one of the checks failing - doesn’t seem related to changes in this PR. Is this error specific on this branch? |
https://feature-requests.datahubproject.io/p/confluent-s3-sink-connector-as-a-source
Note:
I tried updating the existing test
test_kafka_connect.py
but the fixture that creates the docker-compose setup, kept failing due to timeouts. To work around that, I created an entirely separate test.I understand there is a bit of duplication but if the former approach works in the CI, happy to port my test back into
test_kafka_connect.py
.Checklist