Skip to content

feat: Implement Fs2KafkaModule #246

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

Merged
merged 7 commits into from
May 25, 2020
Merged

feat: Implement Fs2KafkaModule #246

merged 7 commits into from
May 25, 2020

Conversation

jakubjanecek
Copy link
Collaborator

@jakubjanecek jakubjanecek commented May 22, 2020

This PR implements subproject for FS2 Kafka.

Closes #67

@jakubjanecek jakubjanecek requested review from hanny24 and jsilva May 24, 2020 18:12
@jakubjanecek jakubjanecek self-assigned this May 24, 2020
@jakubjanecek jakubjanecek added the feature New feature or request label May 24, 2020
Copy link
Contributor

@hanny24 hanny24 left a comment

Choose a reason for hiding this comment

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

What I don't like about it is a fact that it won't allow you configure parts of the kafka client that are not expected by this code. Imagine a new kafka client is released and it adds a new config option. SST needs to be updated before you'll actually be able to configure that. I think there should be some sort of escape hatch ie using Map[String,String]. Or did I miss something?

@jakubjanecek
Copy link
Collaborator Author

It allows you to "escape" using ConsumerSettings and ProducerSettings from the underlying fs2-kafka client. Of course that means you also have to rewrite the configuration so it's not so easy. But I guess I can add it to the config case class just to be safe.

@jakubjanecek jakubjanecek requested a review from hanny24 May 25, 2020 11:21
Copy link
Contributor

@hanny24 hanny24 left a comment

Choose a reason for hiding this comment

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

It might be a good idea to include link to upstream documentation of configuration options.

Also, it should be reviewed by someone who plans to use this soon, which I'm not.

@jakubjanecek
Copy link
Collaborator Author

I'm planning to use it ;) Thanks, I will add a link to the docs.

@mergify mergify bot merged commit 8c218d9 into master May 25, 2020
@mergify mergify bot deleted the feat/kafka branch May 25, 2020 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Development

Successfully merging this pull request may close these issues.

Add module for Kafka
3 participants