-
Notifications
You must be signed in to change notification settings - Fork 24
Add config parameter support for FsKafka producer config #135
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
Conversation
Looks good As a workaround, you should be able to do:
Which is similar to what the https://github.com/jet/FsKafka/blob/master/src/FsKafka/FsKafka.fs#L94-L95 |
Thank you @bartelink! Great to know that the customize function can be used this way. Was trying to figure that out myself. Will discuss more internally and update here if this PR would be still needed. |
👍 I think the PR still makes sense either way - just didnt want you to be blocked |
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.
Can you also add a line to changelog.md please?
src/Propulsion.Kafka/Producers.fs
Outdated
@@ -14,12 +14,13 @@ type Producer | |||
/// Default: LZ4 | |||
?compression, | |||
// Deprecated; there's a good chance this will be removed | |||
?degreeOfParallelism) = | |||
?degreeOfParallelism, | |||
?config) = |
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.
As its a binary breaking change either way, it probably makes sense to move customize
down here too and add the custom
arg, in other words copy https://github.com/jet/FsKafka/blob/master/src/FsKafka/FsKafka.fs#L67-L72 and forward all three args on L23
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.
Done, made this change.
You're not blocked - but I would add this anyway, the function to copy over the dictionary would be redundant usage. |
Looks good, thanks for your contribution! |
We need Propulsion to publish to secure Kafka clusters. For this, a
IDictionary<string, string> option
needs to be passed to FsKafka's Producer config. If connecting to secure host, this would contain paths of certificate files in the local file system needed for SSL encryption. Hence, adding an optional parameter here in the Producer type's constructor in Propulsion to support this.