You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/31 12:02:20 UTC
[GitHub] [pulsar] Jason918 opened a new issue #13591: [PIP 132] Include message header size when check maxMessageSize for non-batch message on the client side.
Jason918 opened a new issue #13591:
URL: https://github.com/apache/pulsar/issues/13591
## Motivation
Currently, Pulsar client (Java) only checks payload size for max message size validation.
Client throws TimeoutException if we produce a message with too many properties, see [1].
But the root cause is that is trigged TooLongFrameException in broker server.
In this PIP, I propose to include message header size when check maxMessageSize of non-batch messages, this brings the following benefits.
1. Clients can throw InvalidMessageException immediately if properties takes too much storage space.
2. This will make the behaviour consistent with topic level max message size check in broker.
3. Strictly limit the entry size less than maxMessageSize, avoid sending message to bookkeeper failed.
## Goal
Include message header size when check maxMessageSize for non-batch message on the client side.
## Implementation
```
// Add a size check in org.apache.pulsar.client.impl.ProducerImpl#processOpSendMsg
if (op.msg != null // for non-batch messages only
&& op.getMessageHeaderAndPayloadSize() > ClientCnx.getMaxMessageSize()) {
// finish send op with InvalidMessageException
releaseSemaphoreForSendOp(op);
op.sendComplete(new PulsarClientException(new InvalidMessageException, op.sequenceId));
}
// org.apache.pulsar.client.impl.ProducerImpl.OpSendMsg#getMessageHeaderAndPayloadSize
public int getMessageHeaderAndPayloadSize() {
ByteBuf cmdHeader = cmd.getFirst();
cmdHeader.markReaderIndex();
int totalSize = cmdHeader.readInt();
int cmdSize = cmdHeader.readInt();
int msgHeadersAndPayloadSize = totalSize - cmdSize - 4;
cmdHeader.resetReaderIndex();
return msgHeadersAndPayloadSize;
}
```
## Reject Alternatives
Add a new property like "maxPropertiesSize" or "maxHeaderSize" in broker.conf and pass it to client like maxMessageSize.
But the implementation is much more complex, and don't have the benefit 2 and 3 mentioned in Motivation.
## Compatibility Issue
As a matter of fact, this PIP narrows down the sendable range. Previously, when maxMessageSize is 1KB, it's ok to send message with 1KB properties and 1KB payload. But with this PIP, the sending will fail with InvalidMessageException.
One conservative way is to add a boolean config "includeHeaderInSizeCheck" to enable this feature. But I think it's OK to enable this directly as it's more reasonable, and I don't see good migration plan if we add a config for this.
The compatibility issue is worth discussing. And any suggestions are appreciated.
[1] https://github.com/apache/pulsar/issues/13560
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui closed issue #13591: [PIP 132] Include message header size when check maxMessageSize for non-batch message on the client side.
Posted by GitBox <gi...@apache.org>.
codelipenghui closed issue #13591:
URL: https://github.com/apache/pulsar/issues/13591
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on issue #13591: [PIP 132] Include message header size when check maxMessageSize for non-batch message on the client side.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on issue #13591:
URL: https://github.com/apache/pulsar/issues/13591#issuecomment-1052935229
The issue had no activity for 30 days, mark with Stale label.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao removed a comment on issue #13591: [PIP 132] Include message header size when check maxMessageSize for non-batch message on the client side.
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on issue #13591:
URL: https://github.com/apache/pulsar/issues/13591#issuecomment-1004483145
Hi, @Jason918
I think we can deprecate ``maxMessageSize `` to ``maxPayloadSize``, and add ``maxHeaderSize `` to limit header size.
after then, we can use ``maxPayloadSize`` + ``maxHeaderSize`` to get ``maxMessageSize`` at internal.
What do you think about it?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on issue #13591: [PIP 132] Include message header size when check maxMessageSize for non-batch message on the client side.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on issue #13591:
URL: https://github.com/apache/pulsar/issues/13591#issuecomment-1004483145
Hi, @Jason918
I think we can deprecate ``maxMessageSize `` to ``maxPayloadSize``, and add ``maxHeaderSize `` to limit header size.
after then, we can use ``maxPayloadSize`` + ``maxHeaderSize`` to get ``maxMessageSize`` at internal.
What do you think about it?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org