Improve batch message acking by removing batch message tracker by sijie · Pull Request #1424 · apache/pulsar

sijie · 2018-03-23T03:08:57Z

Motivation

client is maintaining a batch message tracker to track batch messages for acknowledge. if the application is using Failover subscription and doesn't ack the messages at all, the message id objects will be accumulating in the client.

Modifications

Move the bitset as part of BatchMessageIdImpl and remove batch message tracker

merlimat

Change looks good, refactoring that logic into own class was long overdue. I think there is a part missing when handling cumulative acks and there seems to be legit test failures.

merlimat · 2018-03-23T16:51:09Z

+    }
+
+    // bitset shared across messages in the same batch.
+    private final BitSet bitSet;


Since we have many of these objects, we could inherit from BitSet to avoid an extra object.

hmm then I have to change how BatchMessageAckerDisabled and such. I think this change here removes the hashmap, which already remove bunch of allocations per batch message. It might be better to hide bitset behind the acker and make batch message acker a recycle object, which we can reuse those objects.

Sure, we can do that later

merlimat · 2018-03-23T16:56:45Z

-        if (batchMessageAckTracker.isEmpty()) {
-            return;
-        }
-        MessageIdImpl lowerKey = batchMessageAckTracker.lowerKey(message);


I don't see how this logic to remove all entries from previous batches (when doing cumulative ack) has been replaced.

@merlimat the first PR has a wrong logic on cumulative ack. I fixed it at ceb7b45

merlimat

👍 Nice improvement!

sijie · 2018-03-26T18:18:30Z

retest this please

merlimat · 2018-03-26T23:54:28Z

@sijie can you merge with master? It already has the flaky test fixed.

…atch_messages

sijie · 2018-03-27T00:09:26Z

merged latest master

merlimat · 2018-03-27T23:02:03Z

@sijie I think there might be a legitimate test failure: https://builds.apache.org/job/pulsar_precommit_java8/55/testReport/junit/org.apache.pulsar.client.api/SimpleProducerConsumerStatTest/testMessageListener/

sijie · 2018-03-28T02:21:55Z

@merlimat will take a look

…atch_messages

sijie · 2018-03-28T05:43:56Z

@merlimat the problem is on getting batch size. fixed it at 42fa9e0

Fixes apache#130 ### Motivation It's a catchup for apache/pulsar#1424 ### Modifications Migrate `BitSet` implementation from JDK. Though we have `vector<bool>` or `boost::dynamic_bitset`, to support batch index ACK in future, it's better to have the same method to convert a bit set to the underlying long array. Add `BatchedMessageIdImpl` to maintain a `BatchMessageAcker`, which is shared by messages in the same batch. The acker is responsible to record which messages are acknowledged. Only if all messages in the batch are acknowledged will the message id be acknowledged. For cumulative ack, use the number of removed batches to update the stats. (TODO: record the batch size in the unacked message tracker in future).

Fixes apache#130 ### Motivation It's a catchup for apache/pulsar#1424 ### Modifications Migrate `BitSet` implementation from JDK. Though we have `vector<bool>` or `boost::dynamic_bitset`, to support batch index ACK in future, it's better to have the same method to convert a bit set to the underlying long array. Add `BatchedMessageIdImpl` to maintain a `BatchMessageAcker`, which is shared by messages in the same batch. The acker is responsible to record which messages are acknowledged. Only if all messages in the batch are acknowledged will the message id be acknowledged.

Fixes apache#130 ### Motivation It's a catchup for apache/pulsar#1424 ### Modifications Migrate `BitSet` implementation from JDK. Though we have `vector<bool>` or `boost::dynamic_bitset`, to support batch index ACK in future, it's better to have the same method to convert a bit set to the underlying long array. Add `BatchedMessageIdImpl` to maintain a `BatchMessageAcker`, which is shared by messages in the same batch. The acker is responsible to record which messages are acknowledged. Only if all messages in the batch are acknowledged will the message id be acknowledged. The stats for individual ACKs only updates after the whole batch are acknowledged. Before this PR, each time the single message is acknowledged, the stats increase by one.

* Add BatchedMessageIdImpl to acknowledge batched messages Fixes #130 ### Motivation It's a catchup for apache/pulsar#1424 ### Modifications Migrate `BitSet` implementation from JDK. Though we have `vector<bool>` or `boost::dynamic_bitset`, to support batch index ACK in future, it's better to have the same method to convert a bit set to the underlying long array. Add `BatchedMessageIdImpl` to maintain a `BatchMessageAcker`, which is shared by messages in the same batch. The acker is responsible to record which messages are acknowledged. Only if all messages in the batch are acknowledged will the message id be acknowledged. The stats for individual ACKs only updates after the whole batch are acknowledged. Before this PR, each time the single message is acknowledged, the stats increase by one.

Improve batch message acking by removing batch message tracker

8cfb1ac

sijie added area/client type/performance labels Mar 23, 2018

sijie added this to the 2.0.0-incubating milestone Mar 23, 2018

sijie self-assigned this Mar 23, 2018

sijie requested review from merlimat and rdhabalia March 23, 2018 03:08

merlimat reviewed Mar 23, 2018

View reviewed changes

sijie added 2 commits March 23, 2018 16:30

fix

46266ec

Fix the cumulative ack

ceb7b45

merlimat approved these changes Mar 24, 2018

View reviewed changes

merlimat requested a review from jai1 March 24, 2018 05:14

Merge remote-tracking branch 'apache/master' into improve_acking_on_b…

54839b4

…atch_messages

jai1 approved these changes Mar 28, 2018

View reviewed changes

sijie added 2 commits March 27, 2018 22:38

Fix batchsize

42fa9e0

Merge remote-tracking branch 'apache/master' into improve_acking_on_b…

f00acf1

…atch_messages

jai1 approved these changes Mar 28, 2018

View reviewed changes

merlimat merged commit a141f30 into apache:master Mar 28, 2018

merlimat mentioned this pull request Feb 17, 2019

Fixed C++ batch acks tracker to evict message from sendList array #3618

Merged

BewareMyPower mentioned this pull request Nov 30, 2022

[Bug] Memory keeps increasing if consumer doe not acknowledge apache/pulsar-client-cpp#130

Closed

2 tasks

BewareMyPower mentioned this pull request Nov 30, 2022

Add BatchedMessageIdImpl to acknowledge batched messages apache/pulsar-client-cpp#132

Merged

BewareMyPower mentioned this pull request Dec 22, 2022

[Bug] Deserialized BatchMessageIdImpl cannot be used for acknowledgment #19030

Open

2 tasks

sijie mentioned this pull request Dec 22, 2022

ISSUE-19030: [Bug] Deserialized BatchMessageIdImpl cannot be used for acknowledgment streamnative/pulsar-archived#5297

Open

2 tasks

BewareMyPower mentioned this pull request Dec 30, 2022

[fix][client] Fix deserialized BatchMessageIdImpl acknowledgment failure #19031

Open

4 tasks

Navigation Menu

Uh oh!

Improve batch message acking by removing batch message tracker - #1424

Improve batch message acking by removing batch message tracker#1424
merlimat merged 6 commits into
apache:masterfrom
sijie:improve_acking_on_batch_messages

sijie commented Mar 23, 2018

Uh oh!

merlimat left a comment

Uh oh!

merlimat Mar 23, 2018

Uh oh!

sijie Mar 23, 2018

Uh oh!

merlimat Mar 23, 2018

Uh oh!

merlimat Mar 23, 2018

Uh oh!

sijie Mar 24, 2018

Uh oh!

merlimat left a comment

Uh oh!

sijie commented Mar 26, 2018

Uh oh!

merlimat commented Mar 26, 2018 •

edited

Loading

Uh oh!

sijie commented Mar 27, 2018

Uh oh!

merlimat commented Mar 27, 2018

Uh oh!

sijie commented Mar 28, 2018

Uh oh!

sijie commented Mar 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

sijie commented Mar 23, 2018

Motivation

Modifications

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

sijie Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

merlimat Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

merlimat Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

sijie Mar 24, 2018

Choose a reason for hiding this comment

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

sijie commented Mar 26, 2018

Uh oh!

merlimat commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sijie commented Mar 27, 2018

Uh oh!

merlimat commented Mar 27, 2018

Uh oh!

sijie commented Mar 28, 2018

Uh oh!

sijie commented Mar 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

merlimat commented Mar 26, 2018 •

edited

Loading