Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
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
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+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.
+1 (non-binding)
On Wed, Jan 12, 2022 at 7:07 PM PengHui Li 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
> wrote:
>
> > +1 (non)
> >
> >
> > On Tue, Jan 11, 2022 at 9:46 PM r...@apache.org >
> > wrote:
> >
> > > +1 (non-binding)
> > >
> > > --
> > > Thanks
> > > Xiaolong Ran
> > >
> > > mattison chao 于2022年1月12日周三 10:15写道：
> > >
> > > > +1 (non-binding)
> > > >
> > > > On Wed, 12 Jan 2022 at 09:59, Zike Yang > .invalid>
> > > > wrote:
> > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <
> > jianghait...@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
> >
> >
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+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
wrote:
> +1 (non)
>
>
> On Tue, Jan 11, 2022 at 9:46 PM r...@apache.org
> wrote:
>
> > +1 (non-binding)
> >
> > --
> > Thanks
> > Xiaolong Ran
> >
> > mattison chao 于2022年1月12日周三 10:15写道：
> >
> > > +1 (non-binding)
> > >
> > > On Wed, 12 Jan 2022 at 09:59, Zike Yang .invalid>
> > > wrote:
> > >
> > > > +1 (non-binding)
> > > >
> > > > On Wed, Jan 12, 2022 at 9:58 AM Haiting Jiang <
> jianghait...@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
>
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+1 (non)
On Tue, Jan 11, 2022 at 9:46 PM r...@apache.org
wrote:
> +1 (non-binding)
>
> --
> Thanks
> Xiaolong Ran
>
> mattison chao 于2022年1月12日周三 10:15写道：
>
> > +1 (non-binding)
> >
> > On Wed, 12 Jan 2022 at 09:59, Zike Yang
> > wrote:
> >
> > > +1 (non-binding)
> > >
> > > On Wed, Jan 12, 2022 at 9:58 AM 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
> > >
> > >
> > >
> > > --
> > > Zike Yang
> > >
> >
>
--
Chris Herzog Messaging Team | O 630 300 7718 | M 815 263 3764 |
www.tibco.com
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+1 (non-binding)
r...@apache.org 于2022年1月12日周三 11:46写道：
> +1 (non-binding)
>
> --
> Thanks
> Xiaolong Ran
>
> mattison chao 于2022年1月12日周三 10:15写道：
>
> > +1 (non-binding)
> >
> > On Wed, 12 Jan 2022 at 09:59, Zike Yang
> > wrote:
> >
> > > +1 (non-binding)
> > >
> > > On Wed, Jan 12, 2022 at 9:58 AM 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
> > >
> > >
> > >
> > > --
> > > Zike Yang
> > >
> >
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+1 (non-binding)
--
Thanks
Xiaolong Ran
mattison chao 于2022年1月12日周三 10:15写道：
> +1 (non-binding)
>
> On Wed, 12 Jan 2022 at 09:59, Zike Yang
> wrote:
>
> > +1 (non-binding)
> >
> > On Wed, Jan 12, 2022 at 9:58 AM 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
> >
> >
> >
> > --
> > Zike Yang
> >
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+1 (non-binding)
On Wed, 12 Jan 2022 at 09:59, Zike Yang
wrote:
> +1 (non-binding)
>
> On Wed, Jan 12, 2022 at 9:58 AM 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
>
>
>
> --
> Zike Yang
>
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+1 (binding)
Best,
Hang
Zike Yang 于2022年1月12日周三 09:59写道：
>
> +1 (non-binding)
>
> On Wed, Jan 12, 2022 at 9:58 AM 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
>
>
>
> --
> Zike Yang
Re: [VOTE] PIP-132: Include message header size when check maxMessageSize of non-batch message on the client side.
+1 (non-binding)
On Wed, Jan 12, 2022 at 9:58 AM 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
--
Zike Yang
[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
