-
Notifications
You must be signed in to change notification settings - Fork 0
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
update hw-kafka-client #172
Conversation
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.
I don't see anything conceptually wrong here, especially with this comment essentially saying that the underlying library obviates the need to really support batch producing anyway.
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.
Sounds good. I was a little nervous about losing the batch produce, so I followed the links. It seems well justified.
Single-message is only an API frontend, internally librdkafka is still batching requests to brokers. And sending a message is never a blocking operation: librdkafka just puts a message into its internal queue and releases control immediately.
I have a fuzzy memory of us having to hold hw-kafka-client
back to avoid shared library issues on build, but I may be making that up. If backend can build its docker images with this in play, then SGTM.
Funny for us to both get to the same comment 😃 |
I guess I'll try a megarepo PR against this change before merging here then. |
Is it desirable to still accumulate all the errors into a single log message, or should they just log one-by-one as they occur? |
I don't remember why it worked like that. I can see how it'd be simpler to just log it as it happens, and I can't imagine why that wouldn't be acceptable or even better. |
Fixes for two breaking changes in
hw-kafka-client
:The
produceMessageBatch
function was removed. We now repeatedly callproduceMessage
instead. I'm not sure if this is a big deal or not; were we relying on the bulk operation for performance or semantics?ProducerRecord
now has aprHeaders
field. I'm not sure what this is for, but setting it tomempty
seems to work.This could set our
hw-kafka-client
lower bound to4.0.4
, but since that was deprecated (perhaps because4.0.3
->4.0.4
was a breaking change violating PVP), the version is now set to at least5
in all of our Stack configurations.Adding a bunch of reviewers because I don't know anything about Kafka and not sure who does.