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

Set delivery callbacks per-message, not per-client #48

Merged
merged 2 commits into from
Sep 20, 2021
Merged

Set delivery callbacks per-message, not per-client #48

merged 2 commits into from
Sep 20, 2021

Conversation

cnweaver
Copy link
Collaborator

The librdkafa author suggests that callbacks should have bound into them the message with which they're associated, which greatly simplifies reacting to network disruption and ensuring mesage delivery, but adc currently forces the use of a single callback for all messages sent with a given Producer. This change relaxes that constraint, enabling full use of the underlying confluent-kafka feature.

Copy link
Contributor

@spenczar spenczar left a comment

Choose a reason for hiding this comment

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

The code seems correct and reasonable. I'm happy to defer to your judgement on whether this change is desirable.

One concern is backwards-compatibility. Since this removes an attribute from the ProducerConfig class, callers will break if they used delivery_callback. But adc-streaming has very few (.... one?) users, so just not worrying about that seems fair, since you can probably be actually sure nothing breaks!

@cnweaver
Copy link
Collaborator Author

Naturally, for my own use the interface change is exactly what I wanted. To avoid breaking other users, it might be possible to keep the old way of specifying the callback for the whole object as a default to be used for invocations of the method which do not specify anything else? It might be a little difficult to express using no callback for a specific call when there is one set for the object, however, and I'm not sure whether the complexity would be worthwhile.

@spenczar
Copy link
Contributor

Yep, I agree that the complexity there seems pretty high and it isn't clear to me either whether it's worth it.

@cnweaver
Copy link
Collaborator Author

Okay, in that case I have no further changes to propose, and consider this ready to merge from my side.

@spenczar spenczar merged commit a3dacca into astronomy-commons:master Sep 20, 2021
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.

2 participants