+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 > > >
