+1 (non)
On Tue, Jan 11, 2022 at 9:46 PM r...@apache.org <ranxiaolong...@gmail.com> wrote: > +1 (non-binding) > > -- > Thanks > Xiaolong Ran > > mattison chao <mattisonc...@gmail.com> 于2022年1月12日周三 10:15写道： > > > +1 (non-binding) > > > > On Wed, 12 Jan 2022 at 09:59, Zike Yang <zky...@streamnative.io.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 <http://www.tibco.com/>
