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 EnableDeliveryReports to KafkaAttribute #179

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

gliljas
Copy link
Contributor

@gliljas gliljas commented Sep 30, 2020

Being able to set EnableDeliveryReports is quite valuable, since the collector uses ProduceAsync behind the scenes. In high volume scenarios, the difference is huge (and the delivery reports are never exposed anyway).

Also, the MaxMessageBytes value was never properly copied.

@gliljas gliljas force-pushed the EnableDeliveryReports branch from 42e6cf1 to f60fb47 Compare September 30, 2020 12:25
@TsuyoshiUshio
Copy link
Contributor

Hi @gliljas
It looks awesome. Thank you for your contributions!
One last request is, could you update the documentation? I mean Readme.md of this repo.

@gliljas gliljas force-pushed the EnableDeliveryReports branch from f60fb47 to 00896d2 Compare October 31, 2020 21:42
@gliljas
Copy link
Contributor Author

gliljas commented Oct 31, 2020

"Documentation" added

ryancrawcour
ryancrawcour previously approved these changes Nov 18, 2020
@gliljas gliljas force-pushed the EnableDeliveryReports branch from 782b084 to 800563e Compare December 4, 2020 10:39
@gliljas gliljas force-pushed the EnableDeliveryReports branch 2 times, most recently from 659999e to 7aaeaa4 Compare July 8, 2022 16:24
@gliljas
Copy link
Contributor Author

gliljas commented Jul 8, 2022

Rebased

@gliljas gliljas force-pushed the EnableDeliveryReports branch 2 times, most recently from f9eacfa to 58f134e Compare November 6, 2022 19:13
@gliljas
Copy link
Contributor Author

gliljas commented Nov 6, 2022

@shrohilla

@gliljas gliljas force-pushed the EnableDeliveryReports branch from 58f134e to 72024a0 Compare November 6, 2022 19:15
@shrohilla
Copy link
Contributor

@gliljas I would recommend to wait for PR. Post that we will look into this.
1 quick question, do you have any metric how this change will impact in huge volume scenario ??

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