-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix unbuffered string error #2
base: main
Are you sure you want to change the base?
Fix unbuffered string error #2
Conversation
@@ -23,7 +23,7 @@ export const start = async ({ clientId, messenger }) => { | |||
console.log(`${clientId} / Producer.Ready`); | |||
|
|||
for await (const { topic, message, options } of messenger()) { | |||
dispatcher.dispatch(topic, message, options); | |||
dispatcher.dispatch(topic, Buffer(JSON.stringify(message)), options); |
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.
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.
Oh I see. Well I made this change because the code that's in main
(and also what was in Docker hub from the "Using the docker-compose of this repo" commands) don't seem to do what's described there, which results in this error when you run the containers:
kafka-producer_1 | @hitmands/[email protected] / Producer.Ready
kafka-producer_1 | @hitmands/[email protected] / Producer.Dispatcher.Dispatch('stub-topic')
kafka-producer_1 | /kafka-producer-stub/node_modules/node-rdkafka/lib/producer.js:139
kafka-producer_1 | this._client.produce(topic, partition, message, key, timestamp, opaque, headers));
kafka-producer_1 | ^
kafka-producer_1 |
kafka-producer_1 | Error: Message must be a buffer or null
kafka-producer_1 | at Producer.produce (/kafka-producer-stub/node_modules/node-rdkafka/lib/producer.js:139:18)
kafka-producer_1 | at Object.dispatch (file:///kafka-producer-stub/lib/dispatcher.mjs:9:21)
kafka-producer_1 | at Producer.<anonymous> (file:///kafka-producer-stub/lib/producer.mjs:26:20)
kafka-producer_1 |
kafka-producer_1 | Node.js v18.4.0
I see you are already aware of the fix and did so in a different place but maybe it just didn't make its way back to main
. No worries.
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 think this is a reasonable change to add, messages must be buffers, hence it makes sense to toBuffer
internally.
I'd suggest we'd use Buffer.from
as factory to create a buffer object out of a string.
@@ -1,4 +1,4 @@ | |||
FROM node:14-alpine | |||
FROM node:18-alpine |
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.
🥳
@@ -2,6 +2,7 @@ version: "3.9" | |||
|
|||
services: | |||
kafka-producer: | |||
# build: . |
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'd suggest we'd avoid commented out code :)
@@ -5,6 +5,7 @@ export const createDispatcher = (clientId, producer) => ({ | |||
{ partition = null, key = null, ts = Date.now() } = {} | |||
) => { | |||
console.log(`${clientId} / Producer.Dispatcher.Dispatch('${topic}')`); | |||
console.log("message", message.toString()); |
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.
This might lead to a very verbose log, we should put it under a debug
condition or something.
if (LOG_LEVEL === 'debug') {
// do verbose logging here
}
This repo seems really handy but I had some trouble getting it running in its default configuration, so I wanted to share what I did to get it running.
Changes:
Before this change:
After this change: