Jason918 commented on code in PR #16196:
URL: https://github.com/apache/pulsar/pull/16196#discussion_r906648231
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -2214,8 +2233,8 @@ private void recoverProcessOpSendMsgFrom(ClientCnx cnx,
MessageImpl from, long e
/**
* Check if final message size for non-batch and non-chunked messages is
larger than max message size.
*/
- public boolean isMessageSizeExceeded(OpSendMsg op) {
- if (op.msg != null && op.totalChunks <= 1) {
+ private boolean isMessageSizeExceeded(OpSendMsg op) {
+ if (op.msg != null && op.totalChunks <= 1 &&
!conf.isEncryptionEnabled()) {
Review Comment:
Disable size check if isChunkingEnabled?
```suggestion
if (op.msg != null && !conf.isChunkingEnabled() &&
!conf.isEncryptionEnabled()) {
```
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -488,6 +488,10 @@ public void sendAsync(Message<?> message, SendCallback
callback) {
return;
}
+ // Update the message metadata before computing the payload chunk size
to avoid a large message cannot be split
+ // into chunks.
+ final long sequenceId = updateMessageMetadata(msgMetadata,
uncompressedSize);
Review Comment:
> But I think the existing changes in this PR make the computation of the
payload chunk size **more accurate**.
+1
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -488,6 +488,10 @@ public void sendAsync(Message<?> message, SendCallback
callback) {
return;
}
+ // Update the message metadata before computing the payload chunk size
to avoid a large message cannot be split
+ // into chunks.
+ final long sequenceId = updateMessageMetadata(msgMetadata,
uncompressedSize);
Review Comment:
> `ProducerImpl#encryptMessage` could update the metadata as well. I think
it's better to skip the message size check when encryption is enabled, because
message encryption could also increase the size of the payload buffer.
This is a little out the scope of this PR. Maybe need more discussion. I am
not sure if the space complexity of the increased size is O(1) compare to the
limitation. If not, I think it's better to check the size at client side.
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -488,6 +488,10 @@ public void sendAsync(Message<?> message, SendCallback
callback) {
return;
}
+ // Update the message metadata before computing the payload chunk size
to avoid a large message cannot be split
+ // into chunks.
+ final long sequenceId = updateMessageMetadata(msgMetadata,
uncompressedSize);
Review Comment:
> IMO, it's also better to make it clear which metadata fields can be set
before checking if the size exceeds the limit.
I think the bottom line is the fields which can be very large must be
included here, like `property`. Other fields won't affect much as they take
constant small spaces.
--
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]
