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/14 13:17:02 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #16605: [WIP][client]Optimize batch one message
AnonHxy opened a new pull request, #16605:
URL: https://github.com/apache/pulsar/pull/16605
### Motivation
* Optimize batch one message
*
### Modifications
* Optimize batch one message
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193286582
/pulsarbot run Pulsar CI / CI - System - Sql (pull_request)
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193485246
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1221308388
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1203508233
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1203660920
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1202214260
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1195434805
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193822096
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] eolivelli commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1216483551
The change overall looks good, very good !
but unfortunately there are still failing tests, we need all tests to pass before committing the patch
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193290518
/pulsarbot run Pulsar CI / Unit-OTHER Tests (pull_request)
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193309263
/pulsarbot run Pulsar CI / Unit-OTHER Tests (pull_request)
--
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
[GitHub] [pulsar] AnonHxy merged pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy merged PR #16605:
URL: https://github.com/apache/pulsar/pull/16605
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r930974415
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TopicReaderTest.java:
##########
@@ -1096,7 +1096,6 @@ public void testHasMessageAvailableWithBatch() throws Exception {
ReaderImpl<byte[]> reader = (ReaderImpl<byte[]>)pulsarClient.newReader().topic(topicName)
.startMessageId(messageId).startMessageIdInclusive().create();
MessageIdImpl lastMsgId = (MessageIdImpl) reader.getConsumer().getLastMessageId();
- assertTrue(messageId instanceof BatchMessageIdImpl);
Review Comment:
this test becomes wrong, the name is `testHasMessageAvailableWithBatch`
we must rework the test in a way that we are producing batch messages
##########
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:
I don't think we have to set this header
##########
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:
I don't think we have to set this header
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerEntryMetadataE2ETest.java:
##########
@@ -237,9 +237,8 @@ public void testBatchMessage() throws Exception {
Assert.assertTrue(entryMetadata.getBrokerTimestamp() >= sendTime);
Assert.assertEquals(entryMetadata.getIndex(), 0);
System.out.println(message.getProperties());
- Assert.assertEquals(Integer.parseInt(message.getProperty(BATCH_HEADER)), 1);
Review Comment:
it looks like we are changing the behaviour of the test,
maybe in the test we have to send 2 messages in order to trigger the creation of a batch message
##########
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:
this test becomes wrong, the name is `testDisableBatching `
we must rework the test in a way that we are producing batch messages
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -425,7 +425,7 @@ CompletableFuture<MessageId> internalSendWithTxnAsync(Message<?> message, Transa
* @param payload
* @return a new payload
*/
- private ByteBuf applyCompression(ByteBuf payload) {
+ protected ByteBuf applyCompression(ByteBuf payload) {
Review Comment:
instead of opening the visibility of this method, maybe it is better to put here in ProducerImpl a method that creates the OpSendMsg.
I am not sure it is the better way, but maybe you take try to see if the code will look better
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r950789165
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TopicReaderTest.java:
##########
@@ -1096,7 +1096,6 @@ public void testHasMessageAvailableWithBatch() throws Exception {
ReaderImpl<byte[]> reader = (ReaderImpl<byte[]>)pulsarClient.newReader().topic(topicName)
.startMessageId(messageId).startMessageIdInclusive().create();
MessageIdImpl lastMsgId = (MessageIdImpl) reader.getConsumer().getLastMessageId();
- assertTrue(messageId instanceof BatchMessageIdImpl);
Review Comment:
> Please change the name of the method then
>
> @codelipenghui FYI
Changed from `testHasMessageAvailableWithBatch` to `testHasMessageAvailable`
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r950789178
##########
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:
Updated
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193265605
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r933181992
##########
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:
can we add a comment in the code ?
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1220846562
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1207429755
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193269873
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1202155042
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r933201996
##########
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:
Updated, have added ~
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r950789224
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdProduce.java:
##########
@@ -147,6 +148,7 @@ public class CmdProduce {
private ClientBuilder clientBuilder;
private Authentication authentication;
private String serviceURL;
+ private List<MessageId> messageIds = new ArrayList<>();
Review Comment:
Fixed
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1203553184
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1202185013
/pulsarbot Pulsar CI / CI - Unit - Brokers - Broker Group 2 (pull_request)
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1203431895
/pulsarbot CI - CPP, Python Tests / System-PULSAR_CONNECTORS_THREAD Tests (pull_request)
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193335840
/pulsarbot run Pulsar CI / Unit-OTHER Tests (pull_request)
--
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
[GitHub] [pulsar] Jason918 commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1321679148
move release/2.11.0 label to https://github.com/apache/pulsar/pull/18548
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r933246604
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TopicReaderTest.java:
##########
@@ -1096,7 +1096,6 @@ public void testHasMessageAvailableWithBatch() throws Exception {
ReaderImpl<byte[]> reader = (ReaderImpl<byte[]>)pulsarClient.newReader().topic(topicName)
.startMessageId(messageId).startMessageIdInclusive().create();
MessageIdImpl lastMsgId = (MessageIdImpl) reader.getConsumer().getLastMessageId();
- assertTrue(messageId instanceof BatchMessageIdImpl);
Review Comment:
> we must rework the test in a way that we are producing batch messages
I think that we should just producing non-batch messages here. Although the method name is `testHasMessageAvailableWithBatch`, from the above comment(Line1093), I think this code block shoud test publishing single mesage with batch enable~
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -425,7 +425,7 @@ CompletableFuture<MessageId> internalSendWithTxnAsync(Message<?> message, Transa
* @param payload
* @return a new payload
*/
- private ByteBuf applyCompression(ByteBuf payload) {
+ protected ByteBuf applyCompression(ByteBuf payload) {
Review Comment:
It will not make the code look better now I think.
Because publishing one message in batch and publish with batch disable will call different method.
As the new pushed commit `Fix out of order problem` in this PR, we should call `OpSendMsg#create(List<MessageImpl<?>>, ByteBufPair, long, SendCallback)` to create OpSendMsg, instead of using `OpSendMsg#create(MessageImpl<?>, ByteBufPair, long, SendCallback)`.
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1203408359
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1203637812
/pulsarbot Pulsar CI / System-SQL Tests (pull_request)
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1216503545
> but unfortunately there are still failing tests, we need all tests to pass before committing the patch
OK @eolivelli . Some of the failing tests are flaky test.
However the "Pulsar CI / CI - System - Sql (pull_request) " always fail in this PR but success in other PRs. It takes me some time to find the bug. Luckly we find the bug and fix it in https://github.com/apache/pulsar/pull/17093. The bug has nothing to do with this PR, but it confuses me the correctness of the modification.
After https://github.com/apache/pulsar/pull/17093 merged, I will run all CI again and continue my work to make sure all tests pass
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1221468109
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1220267003
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]Optimize batch one message
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1190140276
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193150387
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193602885
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1221489379
@eolivelli @codelipenghui @BewareMyPower @lhotari @michaeljmarshall @merlimat @rdhabalia All tests have passed. PTAL ~
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [wip][improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1193258729
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#discussion_r933930478
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TopicReaderTest.java:
##########
@@ -1096,7 +1096,6 @@ public void testHasMessageAvailableWithBatch() throws Exception {
ReaderImpl<byte[]> reader = (ReaderImpl<byte[]>)pulsarClient.newReader().topic(topicName)
.startMessageId(messageId).startMessageIdInclusive().create();
MessageIdImpl lastMsgId = (MessageIdImpl) reader.getConsumer().getLastMessageId();
- assertTrue(messageId instanceof BatchMessageIdImpl);
Review Comment:
Please change the name of the method then
@codelipenghui FYI
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdProduce.java:
##########
@@ -147,6 +148,7 @@ public class CmdProduce {
private ClientBuilder clientBuilder;
private Authentication authentication;
private String serviceURL;
+ private List<MessageId> messageIds = new ArrayList<>();
Review Comment:
This list will become huge when you use this tool in real world.
If you need to catch the messages you are sending you can use mockito and override the 'send' method
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #16605: [improve][client]PIP-189: No batching if only one message in batch
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #16605:
URL: https://github.com/apache/pulsar/pull/16605#issuecomment-1202595231
/pulsarbot run-failure-checks
--
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