Skip to content

fix(consumer): stuck on the batch with zero records length#3221

Merged
dnwe merged 2 commits intoIBM:mainfrom
sterligov:fix-consumer-stuck
Jul 31, 2025
Merged

fix(consumer): stuck on the batch with zero records length#3221
dnwe merged 2 commits intoIBM:mainfrom
sterligov:fix-consumer-stuck

Conversation

@sterligov
Copy link
Copy Markdown
Contributor

@sterligov sterligov commented Jul 12, 2025

This pull request addresses the known issue described in KAFKA-5443, which was initially analyzed in #2053. A prior attempt to fix it was made in #2057, but the fix was applied incorrectly and did not fully resolve the root cause.

I’ve provided a more detailed explanation of the underlying issue and the reasoning behind this fix in this comment.

@sterligov sterligov force-pushed the fix-consumer-stuck branch from 6bcf24e to 7a66726 Compare July 13, 2025 05:21
Copy link
Copy Markdown
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

I suppose, the only feedback I could give is that it would be nice to have something explaining that this field doesn’t actually appear “on the wire”? I mean, that was my first thought, “wait, are we renaming an encoded field in the fetch response block?” But it’s not, so that is why renaming it shouldn’t be a big deal, right?

But also, this is technically changing an exported field on an exported type… 😬 Although, a quick search of github suggests the only people referencing the field are from forks/clones.…

@sterligov sterligov force-pushed the fix-consumer-stuck branch 2 times, most recently from e2fc916 to 533d7a1 Compare July 20, 2025 11:29
@sterligov
Copy link
Copy Markdown
Contributor Author

I suppose, the only feedback I could give is that it would be nice to have something explaining that this field doesn’t actually appear “on the wire”?

I've added a comment explaining this field.

But also, this is technically changing an exported field on an exported type… 😬 Although, a quick search of github suggests the only people referencing the field are from forks/clones.…

In my opinion, changing the name is not a problem, this field is only used in a few places and it was added in a previous PR that solved this problem in a way that was not quite correct.

@sterligov sterligov marked this pull request as ready for review July 20, 2025 12:03
@puellanivis
Copy link
Copy Markdown
Collaborator

We could consider unexporting the name as well? This would make it perfectly clear that it’s not a part of the exported API?

This is computed locally and isn't part of the protocol so shouldn't be
part of the API either. As we're renaming it as part of this PR we may
as well also unexport it at this stage too

Signed-off-by: Dominic Evans <[email protected]>
@dnwe dnwe force-pushed the fix-consumer-stuck branch 2 times, most recently from 3ec27b0 to e55a4e6 Compare July 31, 2025 13:08
@dnwe dnwe added the fix label Jul 31, 2025
@dnwe dnwe merged commit e8329e4 into IBM:main Jul 31, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants