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

Add properties for producer configuration to Output Binding Attribute definition #57

Merged
merged 4 commits into from
Apr 10, 2019

Conversation

brandonh-msft
Copy link
Member

@brandonh-msft brandonh-msft commented Apr 4, 2019

Targeting #11

Would appreciate review of this; since we're just piping them down in to the Kafka ProducerConfig object, I'm not sure there's any additional work needed.

@fbeltrao
Copy link
Contributor

fbeltrao commented Apr 4, 2019

@brandonh-msft the strategy we have adopted to expose Kafka settings was to use the host.json file, mapping to class KafkaOptions. I would advice following the same line for the producer too.

However it limits setting distinct values for each producer. We can revisit the ones we think that should be overwritten on topic/producer level (i.e. Idempotence).

For Idempotence we need to investigate what does it mean for a producer since it is the only place where a fatal, un-recoverable state is reached.

OT: we need to sync the usage of this. Every time you touch a file you removes them. We either remove from all or use always. My recommendation is to adopt whatever WebJobs SDK is using.

@brandonh-msft
Copy link
Member Author

brandonh-msft commented Apr 4, 2019

@brandonh-msft the strategy we have adopted to expose Kafka settings was to use the host.json file, mapping to class KafkaOptions. I would advice following the same line for the producer too.

However it limits setting distinct values for each producer. We can revisit the ones we think that should be overwritten on topic/producer level (i.e. Idempotence).

Yeah I noticed this as well; we had the options thing but I, too, noticed it would be across all producers instead of allowing for customizability... I'm looking to you/@ryancrawcour for final word on this as I'm no expert on what customers desire.

For Idempotence we need to investigate what does it mean for a producer since it is the only place where a fatal, un-recoverable state is reached.

Again I'll look to you/Ryan for expertise on this. What do consumers expect? What makes sense in Kafka-land?

OT: we need to sync the usage of this. Every file you touch removes them. We either remove from all or use always. My recommendation is to adopt whatever WebJobs SDK is using.

Understood, my code followed traditional .Net rules but I have been working to "undo" these changes when I catch them. feel free to block my PRs on it. We could/should use StyleCop to enforce this - I saw the support was added to the project, but it must not enforce the rule one way or another.

@brandonh-msft brandonh-msft force-pushed the brandonh/fix-11 branch 2 times, most recently from 1fe54b7 to 3275ce7 Compare April 4, 2019 19:46
@ryancrawcour
Copy link
Contributor

We could/should use StyleCop to enforce this

Agreed. We should be consistent one way or the other. And yeah, StyleCop should enforce the rule whatever we pick.

@ryancrawcour
Copy link
Contributor

Ok, for all producers vs particular producers, how do other bindings work? Does EventHubs binding have a config that applies to all producers, or allow you to customize each? I would imagine that having some defaults in host.json works and makes sense, but then allowing you to override that at a single implementation level would be the way to go.

does that make sense here?

@ryancrawcour
Copy link
Contributor

I don't really understand the idempotence:true setting on the producer in all honesty.
Does this mean that this is a setting that devs using our producer (i.e. Function binding) would want to set to control some behavior? If it's not set, what happens?

@brandonh-msft brandonh-msft marked this pull request as ready for review April 9, 2019 22:46
@ryancrawcour ryancrawcour self-requested a review April 10, 2019 05:09
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.

None yet

3 participants