[fix][client] Fix building broken batched message when publishing #24061
Conversation
poorbarcode
added
type/bug
poorbarcode
requested review from
merlimat,
lhotari,
Technoboy-,
codelipenghui,
gaoran10 and
shibd
|
@poorbarcode Please add the following content to your PR description and select a checkbox: |
github-actions
bot
added
doc-not-needed
| for (int j = 0; j <= i; j++) { | ||
| MessageImpl<?> previousMsg = messages.get(j); | ||
| previousMsg.getDataBuffer().resetReaderIndex(); | ||
| } |
There was a problem hiding this comment.
Just wondering why it wouldn't be fine to reset the reader index of all already batched messages? I guess we don't have a test case to fully cover it where it matters?
Would it be possible to add a test case where multiple messages are sent and the failure happens on the last one? To ensure that all messages get sent successfully when the sending resumes.
There was a problem hiding this comment.
I described this in the Motivation section
(Highlight): BTW, since {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}]
The message-payload will be read again once the previous Send-Command building fails; it needs to be reset after a failed Send-Command building.
I reset the message payload after reading it, which is a simple way to fix the issue of reading an empty payload. Another solution is as follows, which is more safe:
- records
message-payload.readerIndexwhen messages are added - only reset
message-payloadwhen failed building a Send-Command
Please show your view that prefer which soltion
There was a problem hiding this comment.
@poorbarcode Please add a similar test case as what you have already added with the variation that instead of failing after every message, fail and flushAsync only after the 3rd message. That would reveal the problem if it exists.
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.16% +0.59%
+ Complexity 32624 32025 -599
============================================
Files 1877 1862 -15
Lines 139502 144231 +4729
Branches 15299 16432 +1133
============================================
+ Hits 102638 106970 +4332
+ Misses 28908 28816 -92
- Partials 7956 8445 +489
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Motivation
1. The views of the issue:
ava.lang.IllegalArgumentException: Invalid unknown tag type: 3testNoEnoughMemSend2. The steps of issue occurs.
1msg in the container, the batched message will be built as[{msg-metadata-1}, {msg-payload}].OpSendMsgBatchMessageContainerImpl.batchedMessageMetadataAndPayloadis[{msg-metadata-1}, {msg-payload-1}]now{batch-msg-metadata}will not be appended intoBatchMessageContainerImpl.batchedMessageMetadataAndPayload, the variable will be[{msg-payload-1}]2msg 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.batchedMessageMetadataAndPayloadalready 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-3will be discarded because the value ofbatch-metadata.numChunksFromMsgis3, but there are5single message metadata.4. Error that we encountered
bin/pulsar-admin topics get-message-by-id -l 2310013 -e 57 <topic name>Modifications
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: x