lhotari commented on code in PR #24061:
URL: https://github.com/apache/pulsar/pull/24061#discussion_r1983895152
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java:
##########
@@ -162,13 +162,11 @@ protected ByteBuf
getCompressedBatchMetadataAndPayload(boolean clientOperation)
} catch (Throwable th) {
// serializing batch message can corrupt the index of message
and batch-message. Reset the index so,
// next iteration doesn't send corrupt message to broker.
- for (int j = 0; j <= i; j++) {
- MessageImpl<?> previousMsg = messages.get(j);
- previousMsg.getDataBuffer().resetReaderIndex();
- }
Review 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.
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -2353,7 +2353,9 @@ private void batchMessageAndSend(boolean
shouldScheduleNextBatchFlush) {
processOpSendMsg(opSendMsg);
}
} catch (Throwable t) {
- log.warn("[{}] [{}] error while create opSendMsg by batch
message container", topic, producerName, t);
+ batchMessageContainer.resetPayloadAfterFailedPublishing();
+ log.warn("[{}] [{}] error while create opSendMsg by batch
message container, and reset payloads",
Review Comment:
The original warning message was vague and the warning message remains
vague. This is an opportunity to provide Pulsar users with clear, actionable
information.
Consider following Google's error message guideline: after explaining the
cause of the problem, explain how to fix the problem.
- [Identify the
cause](https://developers.google.com/tech-writing/error-messages/identify-the-cause)
- "Tell users exactly what went wrong. Be specific—vague error messages
frustrate users."
- [Explain how to fix the
problem](https://developers.google.com/tech-writing/error-messages/show-fix)
- "Create actionable error messages."
For example "Failed to create batch message for sending. Batch payloads have
been reset and messages will be retried in subsequent batches." (Claude AI's
suggestion based on the PR details)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
