[improve][broker] Make MessageRedeliveryController work more efficiently #17804

codelipenghui · 2022-09-22T12:56:01Z

Motivation

The implementation of MessageRedeliveryController is inefficient

Duplicated Ledger IDs https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentSortedLongPairSet.java#L50
high sorting overhead even if you just want to get a few redeliveries https://github.com/apache/pulsar/pull/17804/files#diff-4c2d35368b21e5c33d65886196e8673c04a69f5c5cb888cdd04c29f8823502fdL109-L122

"BookKeeperClientWorker-OrderedExecutor-1-0" #40 prio=5 os_prio=0 cpu=27401742.31ms elapsed=788038.50s tid=0x00007f4cf95bc800 nid=0x56 runnable  [0x00007f4cd49a0000]
   java.lang.Thread.State: RUNNABLE
	at java.util.TreeMap.put(java.base@11.0.15/TreeMap.java:566)
	at java.util.TreeSet.add(java.base@11.0.15/TreeSet.java:255)
	at org.apache.pulsar.common.util.collections.ConcurrentSortedLongPairSet.lambda$items$4(ConcurrentSortedLongPairSet.java:152)
	at org.apache.pulsar.common.util.collections.ConcurrentSortedLongPairSet$$Lambda$1150/0x00000008408d9040.accept(Unknown Source)
	at org.apache.pulsar.common.util.collections.ConcurrentLongPairSet$Section.forEach(ConcurrentLongPairSet.java:563)
	at org.apache.pulsar.common.util.collections.ConcurrentLongPairSet.forEach(ConcurrentLongPairSet.java:242)
	at org.apache.pulsar.common.util.collections.ConcurrentSortedLongPairSet.items(ConcurrentSortedLongPairSet.java:151)
	at org.apache.pulsar.common.util.collections.ConcurrentSortedLongPairSet.items(ConcurrentSortedLongPairSet.java:143)
	at org.apache.pulsar.common.util.collections.ConcurrentSortedLongPairSet.items(ConcurrentSortedLongPairSet.java:128)
	at org.apache.pulsar.broker.service.persistent.MessageRedeliveryController.getMessagesToReplayNow(MessageRedeliveryController.java:112)
	at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.getMessagesToReplayNow(PersistentDispatcherMultipleConsumers.java:850)
	- eliminated <0x0000000757da7548> (a org.apache.pulsar.broker.service.persistent.PersistentStickyKeyDispatcherMultipleConsumers)
	at org.apache.pulsar.broker.service.persistent.PersistentStickyKeyDispatcherMultipleConsumers.getMessagesToReplayNow(PersistentStickyKeyDispatcherMultipleConsumers.java:432)
	- locked <0x0000000757da7548> (a org.apache.pulsar.broker.service.persistent.PersistentStickyKeyDispatcherMultipleConsumers)
	at org.apache.pulsar.broker.service.persistent.PersistentStickyKeyDispatcherMultipleConsumers.sendMessagesToConsumers(PersistentStickyKeyDispatcherMultipleConsumers.java:175)
	at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.readEntriesComplete(PersistentDispatcherMultipleConsumers.java:483)
	- locked <0x0000000757da7548> (a org.apache.pulsar.broker.service.persistent.PersistentStickyKeyDispatcherMultipleConsumers)
	at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$11.readEntryComplete(ManagedCursorImpl.java:1326)
	- locked <0x00000007129d8210> (a org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$11)
	at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.lambda$asyncReadEntry0$0(EntryCacheImpl.java:222)
	at org.apache.bookkeeper.mledger.impl.EntryCacheImpl$$Lambda$1158/0x00000008408db440.accept(Unknown Source)
	at java.util.concurrent.CompletableFuture$UniAccept.tryFire(java.base@11.0.15/CompletableFuture.java:714)
	at java.util.concurrent.CompletableFuture$Completion.run(java.base@11.0.15/CompletableFuture.java:478)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@11.0.15/ThreadPoolExecutor.java:1128)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@11.0.15/ThreadPoolExecutor.java:628)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.lang.Thread.run(java.base@11.0.15/Thread.java:829)

The issue usually happens if you have a large number of backlogs with the Key_Shared subscription. The publish latency will go high due to the IO thread and BookKeeperClientWorker thread with high high CPU usage

A related fix #15354, but the fix can't fix the case that the MessageRedeliveryController have many redelivery messages
This PR can make the MessageRedeliveryController work more efficiently by introducing a new Ordered Map and Bitmap based LongPair Ordered Set

And the issue can be reproduced by standalone and pulsar-perf

Start a standalone
Use pulsar-perf to produce messages bin/pulsar-perf produce test -r 1000 -bm 1 -mk random
Open 3 terminals to run bin/pulsar-perf consume -st Key_Shared -r 100 -n 50 test
After a while, stop 2 perf consume process
You will see the publish latency increased

With this PR, the situation will not happen

Modifications

Added RoaringBitmap dependency which is a widely used compressed bitset. The dependency is introduced to broker, not the common module because the common module will also expose the Java Client
Added BitmapSortedLongPairSet for MessageRedeliveryController
Update the MessageRedeliveryController to use BitmapSortedLongPairSet for messagesToRedeliver

Verifying this change

New test added BitmapSortedLongPairSetTest

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc-required
(Your PR needs to update docs, and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

Matching PR in forked repository

PR in forked repository: codelipenghui#14

merlimat

👍 Nice!

mattisonchao · 2022-09-23T01:12:10Z

pulsar-broker/src/main/java/org/apache/pulsar/utils/BitmapSortedLongPairSet.java

+        }
+    }
+
+    public synchronized void removeUpTo(long item1, long item2) {


Question: why do we need synchronized here?

😁, I copied the method signature from another class

mattisonchao · 2022-09-23T01:29:55Z

pulsar-broker/src/main/java/org/apache/pulsar/utils/BitmapSortedLongPairSet.java

+import org.apache.pulsar.common.util.collections.LongPairSet;
+import org.roaringbitmap.RoaringBitmap;
+
+public class BitmapSortedLongPairSet {


Maybe we can indicate this class is thread-safe by adding a comment or name like Concurrent***?

315157973 · 2022-09-23T01:47:15Z

pulsar-broker/src/main/java/org/apache/pulsar/utils/BitmapSortedLongPairSet.java

+            Map.Entry<Long, RoaringBitmap> firstEntry = map.firstEntry();
+            while (firstEntry != null && firstEntry.getKey() <= item1) {
+                if (firstEntry.getKey() < item1) {
+                    map.remove(firstEntry.getKey(), firstEntry.getValue());


Is it possible to avoid loop traversal by using subMap?

Sub map will have the low and upper boundaries. If we change to sub map here e.g. from 3 to 10, you will not able to add 2 or 11 again.

pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java

…tly (#17804) (cherry picked from commit c60f895)

…tly (apache#17804) (cherry picked from commit c60f895) (cherry picked from commit 19eb842)

…tly (apache#17804) (cherry picked from commit c60f895)

…request !71) [improve][broker] Make MessageRedeliveryController work more efficiently (apache#17804)

[improve][broker] Make MessageRedeliveryController work more efficiently

b00bdc6

codelipenghui self-assigned this Sep 22, 2022

codelipenghui added this to the 2.12.0 milestone Sep 22, 2022

codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker doc-not-needed Your PR changes do not impact docs release/2.9.4 release/2.11.1 release/2.10.3 labels Sep 22, 2022

merlimat approved these changes Sep 22, 2022

View reviewed changes

mattisonchao approved these changes Sep 23, 2022

View reviewed changes

mattisonchao reviewed Sep 23, 2022

View reviewed changes

315157973 reviewed Sep 23, 2022

View reviewed changes

Address comments

b234bdb

coderzc reviewed Sep 23, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/utils/ConcurrentBitmapSortedLongPairSet.java Show resolved Hide resolved

lhotari mentioned this pull request Sep 23, 2022

[Tests] LockManagerImpl.close hangs and causes the test run to timeout eventually #13092

Closed

codelipenghui requested review from gaoran10, Technoboy- and congbobo184 September 23, 2022 11:33

coderzc approved these changes Sep 23, 2022

View reviewed changes

315157973 approved these changes Sep 23, 2022

View reviewed changes

codelipenghui merged commit c60f895 into apache:master Sep 23, 2022

codelipenghui added a commit that referenced this pull request Sep 23, 2022

[improve][broker] Make MessageRedeliveryController work more efficien…

cfe2dd4

…tly (#17804) (cherry picked from commit c60f895)

codelipenghui added a commit that referenced this pull request Sep 23, 2022

[improve][broker] Make MessageRedeliveryController work more efficien…

19eb842

…tly (#17804) (cherry picked from commit c60f895)

codelipenghui added a commit that referenced this pull request Sep 23, 2022

[improve][broker] Make MessageRedeliveryController work more efficien…

7045376

…tly (#17804) (cherry picked from commit c60f895)

codelipenghui removed the release/2.11.1 label Sep 23, 2022

codelipenghui modified the milestones: 2.12.0, 2.11.0 Sep 23, 2022

codelipenghui added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 labels Sep 23, 2022

codelipenghui added the cherry-picked/branch-2.11 label Sep 23, 2022

Jason918 added release/2.10.2 and removed release/2.10.3 labels Sep 26, 2022

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 28, 2022

[improve][broker] Make MessageRedeliveryController work more efficien…

ea183f3

…tly (apache#17804) (cherry picked from commit c60f895) (cherry picked from commit 19eb842)

lifepuzzlefun mentioned this pull request Oct 16, 2022

CPU usage 100% when use Key_Shared #15445

Closed

dragonls pushed a commit to dragonls/pulsar that referenced this pull request Oct 21, 2022

[improve][broker] Make MessageRedeliveryController work more efficien…

cd509de

…tly (apache#17804) (cherry picked from commit c60f895)

dragonls pushed a commit to dragonls/pulsar that referenced this pull request Oct 21, 2022

Merge branch 'dev-druidliu-2.9' into 'branch-2.9-wxg-release' (merge …

fdf61f9

…request !71) [improve][broker] Make MessageRedeliveryController work more efficiently (apache#17804)

codelipenghui deleted the penghui/improve-relay-queue branch October 25, 2022 15:21

lhotari mentioned this pull request May 17, 2023

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 #20176

Merged

15 tasks

thetumbled mentioned this pull request Aug 20, 2024

[cleanup][broker] delete ConcurrentSortedLongPairSet. #23202

Merged

15 tasks

lhotari mentioned this pull request Sep 4, 2024

[Bug] A large backlog of Key_Shared subscription messages will result in fullgc and OOM #21045

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] Make MessageRedeliveryController work more efficiently #17804

[improve][broker] Make MessageRedeliveryController work more efficiently #17804

codelipenghui commented Sep 22, 2022 •

edited

Loading

merlimat left a comment

mattisonchao Sep 23, 2022

codelipenghui Sep 23, 2022

mattisonchao Sep 23, 2022

315157973 Sep 23, 2022

codelipenghui Sep 23, 2022

[improve][broker] Make MessageRedeliveryController work more efficiently #17804

[improve][broker] Make MessageRedeliveryController work more efficiently #17804

Conversation

codelipenghui commented Sep 22, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

merlimat left a comment

Choose a reason for hiding this comment

mattisonchao Sep 23, 2022

Choose a reason for hiding this comment

codelipenghui Sep 23, 2022

Choose a reason for hiding this comment

mattisonchao Sep 23, 2022

Choose a reason for hiding this comment

315157973 Sep 23, 2022

Choose a reason for hiding this comment

codelipenghui Sep 23, 2022

Choose a reason for hiding this comment

codelipenghui commented Sep 22, 2022 •

edited

Loading