You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by mattison chao <ma...@gmail.com> on 2022/01/04 04:14:33 UTC
Re: [DISCUSSION] PIP-131: Include message header size when check maxMessageSize of non-batch message on the client side.
Hi, @Jason918 <https://github.com/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?
> On Dec 31, 2021, at 8:05 PM, Haiting Jiang <ji...@apache.org> wrote:
>
> https://github.com/apache/pulsar/issues/13591
>
> Pasted below for quoting convenience.
>
> ——
>
> ## 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
>
> Thanks,
> Haiting Jiang
Re: [DISCUSSION] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Haiting Jiang <ji...@apache.org>.
Hi mattison,
Yes, this is an alternative way to solve the case of properties is too large.
But I think this approach is more complex in coding and introduces new concept and
configs, and I don't see the benefit of limiting the header size and payload size separately.
Thanks,
Haiting Jiang
On 2022/01/04 04:14:33 mattison chao wrote:
> Hi, @Jason918 <https://github.com/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?
>
> > On Dec 31, 2021, at 8:05 PM, Haiting Jiang <ji...@apache.org> wrote:
> >
> > https://github.com/apache/pulsar/issues/13591
> >
> > Pasted below for quoting convenience.
> >
> > ——
> >
> > ## 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
> >
> > Thanks,
> > Haiting Jiang
>
>