You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/07/27 12:59:35 UTC
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
AnonHxy commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r931026850
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java:
##########
@@ -197,6 +197,26 @@ public boolean isMultiBatches() {
@Override
public OpSendMsg createOpSendMsg() throws IOException {
+ if (messages.size() == 1) {
+ MessageImpl<?> msg = messages.get(0);
+ messageMetadata = msg.getMessageBuilder();
+ ByteBuf encryptedPayload = producer.encryptMessage(
+ messageMetadata,
+ producer.applyCompression(msg.getDataBuffer()));
+ if (encryptedPayload.readableBytes() > ClientCnx.getMaxMessageSize()) {
+ discard(new PulsarClientException.InvalidMessageException(
+ "Message size is bigger than " + ClientCnx.getMaxMessageSize() + " bytes"));
+ return null;
+ }
+ ByteBufPair cmd = producer.sendMessage(producer.producerId, messageMetadata.getSequenceId(),
+ 1, messageMetadata, encryptedPayload);
+ final OpSendMsg op;
+ op = OpSendMsg.create(msg, cmd, messageMetadata.getSequenceId(), firstCallback);
+ op.setNumMessagesInBatch(1);
Review Comment:
Same as `OpSendMsg#batchSizeByte` below. But the because the `numMessagesInBatch` default value is `1`, it seems that it doesn't matter if we set it or not.
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java:
##########
@@ -197,6 +197,26 @@ public boolean isMultiBatches() {
@Override
public OpSendMsg createOpSendMsg() throws IOException {
+ if (messages.size() == 1) {
+ MessageImpl<?> msg = messages.get(0);
+ messageMetadata = msg.getMessageBuilder();
+ ByteBuf encryptedPayload = producer.encryptMessage(
+ messageMetadata,
+ producer.applyCompression(msg.getDataBuffer()));
+ if (encryptedPayload.readableBytes() > ClientCnx.getMaxMessageSize()) {
+ discard(new PulsarClientException.InvalidMessageException(
+ "Message size is bigger than " + ClientCnx.getMaxMessageSize() + " bytes"));
+ return null;
+ }
+ ByteBufPair cmd = producer.sendMessage(producer.producerId, messageMetadata.getSequenceId(),
+ 1, messageMetadata, encryptedPayload);
+ final OpSendMsg op;
+ op = OpSendMsg.create(msg, cmd, messageMetadata.getSequenceId(), firstCallback);
+ op.setNumMessagesInBatch(1);
+ op.setBatchSizeByte(encryptedPayload.readableBytes());
Review Comment:
`OpSendMsg#batchSizeByte` will not be serialized to he binnary cmd. It's useful for the `ProducerStats`(line2156), as well as `OpSendMsg#numMessagesInBatch`. So I think we should set it here. @eolivelli
[[ProducerStats](https://github.com/apache/pulsar/blob/5df15dd2edd7eeab309fea35828915c8698ea339/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L2129-L2157)](https://github.com/apache/pulsar/blob/5df15dd2edd7eeab309fea35828915c8698ea339/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L2129-L2157)
##########
pulsar-client-tools-test/src/test/java/org/apache/pulsar/client/cli/PulsarClientToolTest.java:
##########
@@ -261,7 +261,6 @@ public void testDisableBatching() throws Exception {
Assert.assertNotNull(msg);
if (i < numberOfMessages) {
Assert.assertEquals(new String(msg.getData()), "batched");
- Assert.assertTrue(msg.getMessageId() instanceof BatchMessageIdImpl);
Review Comment:
OK, I will rework this test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org