KAFKA-7501: fix producer batch double deallocation when receiving message too large error on expired batch#5807
Conversation
|
retest this please |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Nice catch and thanks for including the test. I left one suggestion to improve the readability of the code (existing issue) and maybe we can take the chance to remove the unused expiryErrorMessage in ProducerBatch.
There was a problem hiding this comment.
Can we please add a method isDone which is basically finalState() != null) and then use it here and everywhere else we do batch.finalState() == null? It would be much easier to understand.
|
@becketqin if you have time, please take a look. |
lindong28
left a comment
lindong28
left a comment
There was a problem hiding this comment.
The patch LGTM after addressing Ismael's comment.
lindong28
left a comment
lindong28
left a comment
There was a problem hiding this comment.
Thanks for the fix! LGTM after addressing Ismael's comment.
lindong28
left a comment
lindong28
left a comment
There was a problem hiding this comment.
Thanks for the fix! LGTM after addressing Ismael's comment.
lindong28
left a comment
lindong28
left a comment
There was a problem hiding this comment.
Thanks for the fix! LGTM after addressing Ismael's comment.
lindong28
left a comment
lindong28
left a comment
There was a problem hiding this comment.
Thanks for the fix! LGTM after addressing Ismael's comment.
…sage too large error on expired batch Kafka will try to deallocate a batch twice and throw IllegalStateException when a "MESSAGE_TOO_LARGE" response arrived after the inflight batch is expired. This patch fixes the issue.
ijuma
force-pushed
the
doubleDeallocation
branch
from
8114bd6 to
0f02236
Compare
|
I pushed a commit to the upstream branch with the suggested changes: xiowu0@0f02236 Not sure why it's not showing in the PR though. |
lindong28
left a comment
lindong28
left a comment
There was a problem hiding this comment.
Thanks for the PR! LGTM after Ismael's comment is addressed.
|
GitHub is having some issues according to their status page, but my commit is now showing. @lindong28, does it look good to you? |
…sage too large error on expired batch (#5807) Minor clean-ups for clarity included. Reviewers: Dong Lin <lindong28@gmail.com>, Ismael Juma <ismael@juma.me.uk>
|
JDK 8 build passed, failures in JDK 11 build are unrelated. Merging to trunk and 2.1 branches. I originally planned to cherry-pick to 2.0, but there were some conflicts, so decided against it. |
|
Thanks for review @lindong28 @ijuma. I don't have a chance to make the suggested change during weekend, thanks for the minor tweak @ijuma. |
|
No worries! I did that because I wanted to include it in the upcoming 2.1.0 RC. :) |
…sage too large error on expired batch (apache#5807) Minor clean-ups for clarity included. Reviewers: Dong Lin <lindong28@gmail.com>, Ismael Juma <ismael@juma.me.uk>
…sage too large error on expired batch (apache#5807) Minor clean-ups for clarity included. Reviewers: Dong Lin <lindong28@gmail.com>, Ismael Juma <ismael@juma.me.uk>
3 participants
Kafka will try to deallocate a batch twice and throw IllegalStateException when a "MESSAGE_TOO_LARGE" response arrived after the inflight batch is expired. This patch fixes the issue.
Committer Checklist (excluded from commit message)