You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Haiting Jiang <ji...@apache.org> on 2022/01/12 01:57:59 UTC
[VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
This is the voting thread for PIP-132. It will stay open for at least 48 hours.
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.
[1] https://github.com/apache/pulsar/issues/13560
Thanks,
Haiting Jiang
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Aloys Zhang <al...@apache.org>.
+1 (non-binding)
rxl@apache.org <ra...@gmail.com> 于2022年1月12日周三 11:46写道：
> +1 (non-binding)
>
> --
> Thanks
> Xiaolong Ran
>
> mattison chao <ma...@gmail.com> 于2022年1月12日周三 10:15写道：
>
> > +1 (non-binding)
> >
> > On Wed, 12 Jan 2022 at 09:59, Zike Yang <zk...@streamnative.io.invalid>
> > wrote:
> >
> > > +1 (non-binding)
> > >
> > > On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <jianghaiting@apache.org
> >
> > > wrote:
> > > >
> > > > This is the voting thread for PIP-132. It will stay open for at least
> > 48
> > > hours.
> > > >
> > > > 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.
> > > >
> > > > [1] https://github.com/apache/pulsar/issues/13560
> > > >
> > > > Thanks,
> > > > Haiting Jiang
> > >
> > >
> > >
> > > --
> > > Zike Yang
> > >
> >
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Neng Lu <fr...@gmail.com>.
+1 (non-binding)
On Wed, Jan 12, 2022 at 7:07 PM PengHui Li <pe...@apache.org> wrote:
> +1 (binding)
>
> This is a behavior change, which we should highlight in the release note.
>
> Penghui
>
> On Thu, Jan 13, 2022 at 12:44 AM Chris Herzog <ch...@tibco.com.invalid>
> wrote:
>
> > +1 (non)
> >
> >
> > On Tue, Jan 11, 2022 at 9:46 PM rxl@apache.org <ranxiaolong716@gmail.com
> >
> > wrote:
> >
> > > +1 (non-binding)
> > >
> > > --
> > > Thanks
> > > Xiaolong Ran
> > >
> > > mattison chao <ma...@gmail.com> 于2022年1月12日周三 10:15写道：
> > >
> > > > +1 (non-binding)
> > > >
> > > > On Wed, 12 Jan 2022 at 09:59, Zike Yang <zkyang@streamnative.io
> > .invalid>
> > > > wrote:
> > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <
> > jianghaiting@apache.org
> > > >
> > > > > wrote:
> > > > > >
> > > > > > This is the voting thread for PIP-132. It will stay open for at
> > least
> > > > 48
> > > > > hours.
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > [1] https://github.com/apache/pulsar/issues/13560
> > > > > >
> > > > > > Thanks,
> > > > > > Haiting Jiang
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Zike Yang
> > > > >
> > > >
> > >
> >
> >
> > --
> >
> >
> > Chris Herzog Messaging Team | O 630 300 7718 | M 815 263 3764 |
> > www.tibco.com
> >
> > <http://www.tibco.com/>
> >
>
--
Best Regards,
Neng
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by PengHui Li <pe...@apache.org>.
+1 (binding)
This is a behavior change, which we should highlight in the release note.
Penghui
On Thu, Jan 13, 2022 at 12:44 AM Chris Herzog <ch...@tibco.com.invalid>
wrote:
> +1 (non)
>
>
> On Tue, Jan 11, 2022 at 9:46 PM rxl@apache.org <ra...@gmail.com>
> wrote:
>
> > +1 (non-binding)
> >
> > --
> > Thanks
> > Xiaolong Ran
> >
> > mattison chao <ma...@gmail.com> 于2022年1月12日周三 10:15写道：
> >
> > > +1 (non-binding)
> > >
> > > On Wed, 12 Jan 2022 at 09:59, Zike Yang <zkyang@streamnative.io
> .invalid>
> > > wrote:
> > >
> > > > +1 (non-binding)
> > > >
> > > > On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <
> jianghaiting@apache.org
> > >
> > > > wrote:
> > > > >
> > > > > This is the voting thread for PIP-132. It will stay open for at
> least
> > > 48
> > > > hours.
> > > > >
> > > > > 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.
> > > > >
> > > > > [1] https://github.com/apache/pulsar/issues/13560
> > > > >
> > > > > Thanks,
> > > > > Haiting Jiang
> > > >
> > > >
> > > >
> > > > --
> > > > Zike Yang
> > > >
> > >
> >
>
>
> --
>
>
> Chris Herzog Messaging Team | O 630 300 7718 | M 815 263 3764 |
> www.tibco.com
>
> <http://www.tibco.com/>
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Chris Herzog <ch...@tibco.com.INVALID>.
+1 (non)
On Tue, Jan 11, 2022 at 9:46 PM rxl@apache.org <ra...@gmail.com>
wrote:
> +1 (non-binding)
>
> --
> Thanks
> Xiaolong Ran
>
> mattison chao <ma...@gmail.com> 于2022年1月12日周三 10:15写道：
>
> > +1 (non-binding)
> >
> > On Wed, 12 Jan 2022 at 09:59, Zike Yang <zk...@streamnative.io.invalid>
> > wrote:
> >
> > > +1 (non-binding)
> > >
> > > On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <jianghaiting@apache.org
> >
> > > wrote:
> > > >
> > > > This is the voting thread for PIP-132. It will stay open for at least
> > 48
> > > hours.
> > > >
> > > > 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.
> > > >
> > > > [1] https://github.com/apache/pulsar/issues/13560
> > > >
> > > > Thanks,
> > > > Haiting Jiang
> > >
> > >
> > >
> > > --
> > > Zike Yang
> > >
> >
>
--
Chris Herzog Messaging Team | O 630 300 7718 | M 815 263 3764 |
www.tibco.com
<http://www.tibco.com/>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by "rxl@apache.org" <ra...@gmail.com>.
+1 (non-binding)
--
Thanks
Xiaolong Ran
mattison chao <ma...@gmail.com> 于2022年1月12日周三 10:15写道：
> +1 (non-binding)
>
> On Wed, 12 Jan 2022 at 09:59, Zike Yang <zk...@streamnative.io.invalid>
> wrote:
>
> > +1 (non-binding)
> >
> > On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <ji...@apache.org>
> > wrote:
> > >
> > > This is the voting thread for PIP-132. It will stay open for at least
> 48
> > hours.
> > >
> > > 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.
> > >
> > > [1] https://github.com/apache/pulsar/issues/13560
> > >
> > > Thanks,
> > > Haiting Jiang
> >
> >
> >
> > --
> > Zike Yang
> >
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by mattison chao <ma...@gmail.com>.
+1 (non-binding)
On Wed, 12 Jan 2022 at 09:59, Zike Yang <zk...@streamnative.io.invalid>
wrote:
> +1 (non-binding)
>
> On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <ji...@apache.org>
> wrote:
> >
> > This is the voting thread for PIP-132. It will stay open for at least 48
> hours.
> >
> > 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.
> >
> > [1] https://github.com/apache/pulsar/issues/13560
> >
> > Thanks,
> > Haiting Jiang
>
>
>
> --
> Zike Yang
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Hang Chen <ch...@apache.org>.
+1 (binding)
Best,
Hang
Zike Yang <zk...@streamnative.io.invalid> 于2022年1月12日周三 09:59写道：
>
> +1 (non-binding)
>
> On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <ji...@apache.org> wrote:
> >
> > This is the voting thread for PIP-132. It will stay open for at least 48 hours.
> >
> > 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.
> >
> > [1] https://github.com/apache/pulsar/issues/13560
> >
> > Thanks,
> > Haiting Jiang
>
>
>
> --
> Zike Yang
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Zike Yang <zk...@streamnative.io.INVALID>.
+1 (non-binding)
On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <ji...@apache.org> wrote:
>
> This is the voting thread for PIP-132. It will stay open for at least 48 hours.
>
> 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.
>
> [1] https://github.com/apache/pulsar/issues/13560
>
> Thanks,
> Haiting Jiang
--
Zike Yang
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Lin Lin <li...@apache.org>.
+1
On 2022/01/12 01:57:59 Haiting Jiang wrote:
> This is the voting thread for PIP-132. It will stay open for at least 48 hours.
>
> 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.
>
> [1] https://github.com/apache/pulsar/issues/13560
>
> Thanks,
> Haiting Jiang
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
Posted by Haiting Jiang <ji...@apache.org>.
Thanks for your participation.
Close this vote with 3 (+1) bindings and 6 (+1) non-bindings, 0 (-1).
Thanks,
Haiting
On 2022/01/12 01:57:59 Haiting Jiang wrote:
> This is the voting thread for PIP-132. It will stay open for at least 48 hours.
>
> 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.
>
> [1] https://github.com/apache/pulsar/issues/13560
>
> Thanks,
> Haiting Jiang
>