Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PIP-189: No batching if only one message in batch #16619

Closed
AnonHxy opened this issue Jul 15, 2022 · 3 comments
Closed

PIP-189: No batching if only one message in batch #16619

AnonHxy opened this issue Jul 15, 2022 · 3 comments
Assignees
Labels
Milestone

Comments

@AnonHxy
Copy link
Contributor

AnonHxy commented Jul 15, 2022

discuss mail-thread: https://lists.apache.org/thread/dbq1lrv03bhtk0lr5nwm5txo9ndjplv0

Motivation

The original discussion mail :
https://lists.apache.org/thread/5svpl5qp3bfoztf5fvtojh51zbklcht2

linked issue: #16547

Introduce the ability for producers to publish a non-batched message if there is only one message in the batch.

It is useful to save the SingleMessageMetadata space in entry  and reduce workload of consumers to deserialize the SingleMessageMetadata,  especially  when sometimes there is an amount of batched messages with only one real message.

API Changes

N/A

Implementation

For BatchMessageContainerImpl :

public OpSendMsg createOpSendMsg() throws IOException {
        if (!producer.conf.isBatchingSingleMessage() && messages.size() == 1) {
             // If only one message,  create OpSendMsg as non-batched publish.
        }
        
       // ....
}

For BatchMessageKeyBasedContainer,  there is no need to change, because it uses BatchMessageContainerImpl to create OpSendMsg

Reject Alternatives

  • Always return BatchMessageIdImpl when enableBatching is set as true, even if publish single message  with a non-batched message.
    Rejection reason: Consumer have to deserialize to check if there is SingleMessageMetadata from the payload

  • Add a configuration for the producer to enable or disable this feature
    Rejection reason: It is always a good idea to not create a batch message. So there is no reason to turn off this feature

@eolivelli
Copy link
Contributor

I think that we should not add a configuration flag.
the fact that we are building batch messages or single messages is an implementation detail that is opaque to the Producer and to the Consumers.
Applications should not use Impl classes or count on a specific Impl class.

There is no reason to turn off this feature

@eolivelli
Copy link
Contributor

@AnonHxy did you see my comment ?

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jul 22, 2022

@AnonHxy did you see my comment ?

Yes. I saw the comment. And I had replied from the email. Maybe I should attatch the link to this issue. :)
https://lists.apache.org/thread/dbq1lrv03bhtk0lr5nwm5txo9ndjplv0

I agree with your point. And I have updated the PR to remove the configuration flag. But there is still some flaky-test failure and I am working on it. Thanks @eolivelli

@codelipenghui codelipenghui added this to the 2.12.0 milestone Aug 11, 2022
AnonHxy added a commit that referenced this issue Aug 24, 2022
…16605)

[improve][client]PIP-189: No batching if only one message in batch #16605

### Motivation

* See #16619

### Modifications

* See #16619
* Most of the Modifications are relevant to `BatchMessageContainerImpl`
* Of course there are some tests about batching need to be modified, because batched producer can also pubulish non-batched messages when this PIP applies. The tests include:
    * `RGUsageMTAggrWaitForAllMsgsTest`
    * `BatchMessageTest`
    * `BrokerEntryMetadataE2ETest`
    * `ClientDeduplicationTest`
    * `TopicReaderTest`
    * `PulsarClientToolTest`
@AnonHxy AnonHxy closed this as completed Aug 25, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this issue Aug 29, 2022
…pache#16605)

[improve][client]PIP-189: No batching if only one message in batch apache#16605

### Motivation

* See apache#16619

### Modifications

* See apache#16619
* Most of the Modifications are relevant to `BatchMessageContainerImpl`
* Of course there are some tests about batching need to be modified, because batched producer can also pubulish non-batched messages when this PIP applies. The tests include:
    * `RGUsageMTAggrWaitForAllMsgsTest`
    * `BatchMessageTest`
    * `BrokerEntryMetadataE2ETest`
    * `ClientDeduplicationTest`
    * `TopicReaderTest`
    * `PulsarClientToolTest`
AnonHxy added a commit to AnonHxy/pulsar that referenced this issue Nov 20, 2022
…pache#16605)

[improve][client]PIP-189: No batching if only one message in batch apache#16605

### Motivation

* See apache#16619

### Modifications

* See apache#16619
* Most of the Modifications are relevant to `BatchMessageContainerImpl`
* Of course there are some tests about batching need to be modified, because batched producer can also pubulish non-batched messages when this PIP applies. The tests include:
    * `RGUsageMTAggrWaitForAllMsgsTest`
    * `BatchMessageTest`
    * `BrokerEntryMetadataE2ETest`
    * `ClientDeduplicationTest`
    * `TopicReaderTest`
    * `PulsarClientToolTest`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants