Skip to content

Conversation

@kazanzhy
Copy link
Contributor

@kazanzhy kazanzhy commented Feb 23, 2022

There are a few high-level questions for this integration.

1. How to implement this hook?
The first way is similar to PubSubHook when all functionality is in the hook. Like PubSubHook.publish().
The second way is similar to FirehoseHook when the hook is just a wrapper of the boto client which is used to interact with Kinesis.
Talking about the Apache Pulsar (#21618), it has a good client that manages and reuses sessions for producers and consumers. That's why for Pulsar second way is better.
But for Kafka, there are a few separate classes for producer and consumer.

2. What python package to use?
The kafka-python is more popular, but there are no new commits last 2 years.
On the other hand, confluent-kafka-python is actively developing but
less popular and developing by Confluent company.

@potiuk
Copy link
Member

potiuk commented Feb 25, 2022

@kazanzhy - I think I managed to workaround the pip resolver issue with #21824 - please rebase to latest main.

@kazanzhy kazanzhy force-pushed the add_apache_kafka_hook branch 3 times, most recently from d3e3e16 to e86188b Compare March 2, 2022 15:26
@kazanzhy kazanzhy force-pushed the add_apache_kafka_hook branch 3 times, most recently from 618f1c6 to 7818cea Compare March 10, 2022 12:57
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Can you add simple example dag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is not an Airflow hook.. I think it would be best to use another name to avoid confusion?
Also this class is not covered with unit tests

Copy link
Contributor

Choose a reason for hiding this comment

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

can we test this function?

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 remember we ever edited a UI file when adding a provider?
cc @bbovenzi

Copy link
Contributor

@bbovenzi bbovenzi Apr 14, 2022

Choose a reason for hiding this comment

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

Yeah I would ignore what's in /ui for now. It needs a refresh post 2.3.

@kazanzhy
Copy link
Contributor Author

kazanzhy commented Apr 14, 2022

Hi @eladkal. Thank you for the review
This PR was created mostly for discussion and I really need suggestions to answer the questions in the description.

There are implemented Hooks for PubSub and Kinesis, so I decided to create integrations for Kafka and Pulsar. It will be very convenient if they will be unified.
Therefore I'm not sure if this implementation is right, because of the creation of one more Class to merge Kafka library classes into one "client".

@kazanzhy kazanzhy force-pushed the add_apache_kafka_hook branch from 7818cea to ce7544f Compare May 2, 2022 18:23
@kazanzhy kazanzhy force-pushed the add_apache_kafka_hook branch from ce7544f to 0a97314 Compare May 19, 2022 20:56
@github-actions
Copy link

github-actions bot commented Jul 4, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 4, 2022
@github-actions github-actions bot closed this Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools area:providers area:UI Related to UI/UX. For Frontend Developers. kind:documentation stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants