Skip to content

Conversation

avsaditya19
Copy link
Contributor

@avsaditya19 avsaditya19 commented Jan 27, 2022

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.

@CLAassistant
Copy link

CLAassistant commented Jan 27, 2022

CLA assistant check
All committers have signed the CLA.

@bartelink
Copy link
Collaborator

Looks good

As a workaround, you should be able to do:

let applyConfig (c : ProducerConfig) =
   for KeyValue (k, v) in config do
       c.Set(k, v)

KafkaProducerConfig.Create(
   .....
    customize = applyConfig,
    ...
)

Which is similar to what the config arg achieves in:

https://github.com/jet/FsKafka/blob/master/src/FsKafka/FsKafka.fs#L94-L95

@avsaditya19
Copy link
Contributor Author

Looks good

As a workaround, you should be able to do:

let applyConfig (c : ProducerConfig) =
   for KeyValue (k, v) in config do
       c.Set(k, v)

KafkaProducerConfig.Create(
   .....
    customize = applyConfig,
    ...
)

Which is similar to what the config arg achieves in:

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.

@bartelink
Copy link
Collaborator

👍 I think the PR still makes sense either way - just didnt want you to be blocked

Copy link
Collaborator

@bartelink bartelink 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 also add a line to changelog.md please?

@@ -14,12 +14,13 @@ type Producer
/// Default: LZ4
?compression,
// Deprecated; there's a good chance this will be removed
?degreeOfParallelism) =
?degreeOfParallelism,
?config) =
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, made this change.

@deviousasti
Copy link
Member

You're not blocked - but I would add this anyway, the function to copy over the dictionary would be redundant usage.
Just add a line about the change to the Unreleased section in CHANGELOG.md.

@bartelink
Copy link
Collaborator

Looks good, thanks for your contribution!

@bartelink bartelink merged commit eda4137 into jet:master Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants