-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[fix][client] Fix building broken batched message when publishing #24061
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][client] Fix building broken batched message when publishing #24061
Conversation
@poorbarcode Please add the following content to your PR description and select a checkbox:
|
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.
Good work tracing down this issue, Yubiao! I have some initial feedback as comments.
pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24061 +/- ##
============================================
+ Coverage 73.57% 74.23% +0.65%
+ Complexity 32624 32041 -583
============================================
Files 1877 1862 -15
Lines 139502 144232 +4730
Branches 15299 16433 +1134
============================================
+ Hits 102638 107066 +4428
+ Misses 28908 28712 -196
- Partials 7956 8454 +498
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
@lhotari Could you take a look again? |
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.
LGTM, good work Yubiao!
…ache#24061) (cherry picked from commit c75018a) (cherry picked from commit 038810d)
…ache#24061) (cherry picked from commit c75018a) (cherry picked from commit 038810d)
…ache#24061) (cherry picked from commit c75018a)
…ache#24061) (cherry picked from commit c75018a) (cherry picked from commit ab53f2f)
…ache#24061) (cherry picked from commit c75018a) (cherry picked from commit ab53f2f)
Motivation
1. The views of the issue:
ava.lang.IllegalArgumentException: Invalid unknown tag type: 3
testNoEnoughMemSend
2. The steps of issue occurs.
1
msg in the container, the batched message will be built as[{msg-metadata-1}, {msg-payload}]
.OpSendMsg
BatchMessageContainerImpl.batchedMessageMetadataAndPayload
is[{msg-metadata-1}, {msg-payload-1}]
now{batch-msg-metadata}
will not be appended intoBatchMessageContainerImpl.batchedMessageMetadataAndPayload
, the variable will be[{msg-payload-1}]
2
msg in the container, the batched message will be built as[{batch-metadata}, {single-msg-metadata-1}, {msg-payload-1}, {single-msg-metadata-2}, {msg-payload-2}]
.BatchMessageContainerImpl.batchedMessageMetadataAndPayload
already has some data that has not been cleared, the data actually is[{batch-metadata}, {msg-payload-1}, {single-msg-metadata-1}, {msg-payload-1}, {single-msg-metadata-2}, {msg-payload-2}]
{msg-payload-1}
has been read out at the first flushing, the second one will be empty, so the final data is[{batch-metadata}, {msg-payload-1},{single-msg-metadata-1}, {empty}, {single-msg-metadata-2}, {msg-payload-2}]
3. Explain why it also leads to a message loss issue.
The error will also lead the batch message to be
[{batch-metadata}, {single-msg-metadata-1}, {msg-payload-1}, {single-msg-metadata-2}, {msg-payload-2}, {single-msg-metadata-1}, {empty}, {single-msg-metadata-2}, {empty}, {single-msg-metadata-3}, {msg-payload-3}]
. Then themsg-3
will be discarded because the value ofbatch-metadata.numChunksFromMsg
is3
, but there are5
single message metadata.4. Error that we encountered
bin/pulsar-admin topics get-message-by-id -l 2310013 -e 57 <topic name>
Modifications
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: x