pulsar
pulsar copied to clipboard
apache
[improve][client]PIP-189: No batching if only one message in batch
email discussion thread: https://lists.apache.org/thread/dbq1lrv03bhtk0lr5nwm5txo9ndjplv0
Motivation
- See https://github.com/apache/pulsar/issues/16619
Modifications
- See https://github.com/apache/pulsar/issues/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:
-
BatchMessageTest#testDecreaseUnAckMessageCountWithAckReceipt -
BrokerEntryMetadataE2ETest#testBatchMessage -
ClientDeduplicationTest#testKeyBasedBatchingOrder -
TopicReaderTest#testHasMessageAvailableWithBatch -
PulsarClientToolTest#testDisableBatching
-
Verifying this change
- [x] Make sure that the change passes the CI checks.
- Add UT
org.apache.pulsar.broker.service.BatchMessageTest#testBatchSendOneMessage
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)
The change overall looks good, very good !
but unfortunately there are still failing tests, we need all tests to pass before committing the patch
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 confused me that 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
@eolivelli @codelipenghui @BewareMyPower @lhotari @michaeljmarshall @merlimat @rdhabalia All tests have passed. PTAL ~
