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.

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

Kafka destination #1892

Merged
merged 20 commits into from
Feb 27, 2024
Merged

Kafka destination #1892

merged 20 commits into from
Feb 27, 2024

Conversation

seg-leonelsanches
Copy link
Contributor

This is a basic implementation of a Kafka Destination, using Confluent as the provided. So far, this destination only supports plain auth (username + password, or API Key + Secret). Many things are still stubbed here, since the idea is to gather comments from Eng to improve the code in different iterations.

Testing

I recommend creating a trial account at https://confluent.io, set a sample cluster and follow the steps in the configuration.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [Segmenters] Tested in the staging environment

@seg-leonelsanches seg-leonelsanches requested a review from a team as a code owner February 21, 2024 17:31
@seg-leonelsanches seg-leonelsanches requested a review from a team February 21, 2024 17:31
Copy link
Contributor

@pooyaj pooyaj left a comment

Choose a reason for hiding this comment

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

Looks great 🙌 added few comments

description:
'The brokers for your Kafka instance, in the format of `host:port`. Accepts a comma delimited string.',
type: 'string',
required: true
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, we can do multiple: true instead of asking user to comma separate the brokers

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the review @pooyaj . I had to leave this as a single string as I believe that Destination level settings can only be simple types. The UI doesn't allow for arrays AFAIK. I hope I'm wrong!

@joe-ayoub-segment
Copy link
Contributor

Thanks for the PR review @pooyaj .
Do you have any insights into the design of the Destination? Particularly with regards how partitions are treated, and for the auth mechanism?

@joe-ayoub-segment joe-ayoub-segment enabled auto-merge (squash) February 27, 2024 09:44
@joe-ayoub-segment joe-ayoub-segment merged commit 4a7edef into main Feb 27, 2024
9 checks passed
@joe-ayoub-segment joe-ayoub-segment deleted the kafka branch February 27, 2024 09:44
@joe-ayoub-segment
Copy link
Contributor

PR deployed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants