-
-
Notifications
You must be signed in to change notification settings - Fork 527
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 RecordBatch decode on protocol Fetch v4 #179
Conversation
@tulios Thanks for fixing this quickly, I can confirm that this patch works sometimes! I see a lot of
|
I will take a look; it would help a lot if you could provide us with the Buffer causing the issue. I will try to simulate this today; I have the infra in place now. |
@@ -7,6 +7,7 @@ const { MAGIC_BYTE } = require('../../../recordBatch/v0') | |||
// the magic offset is at the same offset for all current message formats, but the 4 bytes | |||
// between the size and the magic is dependent on the version. | |||
const MAGIC_OFFSET = 16 | |||
const RECORD_BATCH_OVERHEAD = 49 |
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.
Consider if maybe the decoders should expose how many bytes they require, since we have these checks wherever you're decoding a list of something.
@tulios Here's a full stack trace —
|
I will merge this as it solves some issues, but I'll continue investigating the problem |
This bug was making the fetch operation very inefficient. If a RecordBatch had many records, it would work as it should, hence the green tests on KafkaJS. If the messages resulted in many RecordBatch, each with a single record, it would read one message at a time per fetch operation.
This behavior is also breaking snappy and lz4 compressions.
Interesting enough, the response test for fetch v4 had
highWatermark
6, but was checked against 3 items. I guess this slipped through the cracks; after the fix, the test reported 3 other messages missing.Fixes #152