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

update hw-kafka-client #172

Merged
merged 4 commits into from
Jun 7, 2024
Merged

update hw-kafka-client #172

merged 4 commits into from
Jun 7, 2024

Conversation

chris-martin
Copy link
Contributor

@chris-martin chris-martin commented Jun 6, 2024

Fixes for two breaking changes in hw-kafka-client:

  • The produceMessageBatch function was removed. We now repeatedly call produceMessage 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 a prHeaders field. I'm not sure what this is for, but setting it to mempty seems to work.

This could set our hw-kafka-client lower bound to 4.0.4, but since that was deprecated (perhaps because 4.0.3 -> 4.0.4 was a breaking change violating PVP), the version is now set to at least 5 in all of our Stack configurations.

Adding a bunch of reviewers because I don't know anything about Kafka and not sure who does.

Copy link
Contributor

@z0isch z0isch left a 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.

library/Freckle/App/Kafka/Producer.hs Outdated Show resolved Hide resolved
Copy link
Member

@pbrisbin pbrisbin left a 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.

@z0isch
Copy link
Contributor

z0isch commented Jun 6, 2024

well justified.

Funny for us to both get to the same comment 😃

@chris-martin
Copy link
Contributor Author

If backend can build its docker images with this in play

I guess I'll try a megarepo PR against this change before merging here then.

@chris-martin
Copy link
Contributor Author

chris-martin commented Jun 6, 2024

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?

@pbrisbin
Copy link
Member

pbrisbin commented Jun 6, 2024

Is it desirable to still accumulate all the errors into a single log message

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.

@chris-martin chris-martin merged commit cda8ac8 into main Jun 7, 2024
6 checks passed
@chris-martin chris-martin deleted the chris/kafka branch June 7, 2024 00:54
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.

3 participants