[pulsar-broker] Cache unack-messageId into openRangeSet #3819

rdhabalia · 2019-03-14T07:23:32Z

Motivation

This PR is on top of #3818. With this PR changes, ML can use OpenRangeSet to cache unack-messageIds. It also provides option managedLedgerUnackedRangesOpenCacheSetEnabled to switch back to existing RangeSet so, user can flip back to original behavior to avoid any rollback. We will remove managedLedgerUnackedRangesOpenCacheSetEnabled option in future release once it will not be experimental.

Result

Broker will not face gc-pauses in case client generates large number of unack messages into broker.

I performed perf and functional test on the changes:

with existing guava Range Data-structure and large unack msg usecase:

we can see high gc-pause
and over 9M PositionImpl objects
CPU usage around 40%

With OpenRangeSet data-structure and large unack msg usecase:

we can see very low gc-pause
and around 50K PositionImpl objects into memory
and CPU usage increases to 70%

rdhabalia · 2019-04-26T19:34:08Z

rerun java8 tests

rdhabalia · 2019-05-10T21:56:23Z

rerun java8 tests

rdhabalia · 2019-05-10T23:48:57Z

rerun java8 tests

rdhabalia · 2019-05-13T23:18:30Z

rerun java8 tests

rdhabalia · 2019-05-15T01:48:04Z

rerun java8 tests

codelipenghui · 2019-05-17T07:38:29Z

run java8 tests

merlimat · 2019-05-18T14:48:25Z

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryTest.java

 import org.apache.bookkeeper.mledger.ManagedLedgerInfo.CursorInfo;
 import org.apache.bookkeeper.mledger.ManagedLedgerInfo.MessageRangeInfo;
 import org.apache.bookkeeper.test.MockedBookKeeperTestCase;
+import org.assertj.core.util.Lists;


This should be java.utils

merlimat · 2019-05-18T14:49:01Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

 import org.apache.pulsar.common.configuration.FieldContext;
 import org.apache.pulsar.common.configuration.PulsarConfiguration;
 import org.apache.pulsar.common.policies.data.BacklogQuota;
+import org.apache.pulsar.common.util.collections.ConcurrentOpenLongPairRangeSet;


Import shouldn't be needed here

merlimat · 2019-05-18T14:50:17Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/PositionImpl.java

+    protected long ledgerId;
+    protected long entryId;

    public static PositionImpl earliest = new PositionImpl(-1, -1);


Can you also make earliest/latest as final here? (Just happened to realize they were not already...)

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java

merlimat · 2019-05-18T15:01:17Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

    long getNumIndividualDeletedEntriesToSkip(long numEntries) {
-        long totalEntriesToSkip = 0;
-        long deletedMessages = 0;
+        AtomicLong totalEntriesToSkip = new AtomicLong(0);


Instead of the 2 AtomicLong and 2 AtomicReference instances, we could use thread locals here, since it's just to be able to update them from within the lambda.

merlimat · 2019-05-18T15:05:56Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

        this.name = cursorName;
+        this.individualDeletedMessages = config.isUnackedRangesOpenCacheSetEnabled()
+                ? new ConcurrentOpenLongPairRangeSet<PositionImpl>(4096, positionRangeConverter)
+                : new LongPairRangeSet.DefaultRangeSet<>(positionRangeConverter);


Are both cases being tested?

yes..I have tested both.. however, let me data-providers to test both scenarios in existing test cases.

merlimat · 2019-05-18T16:27:31Z

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryTest.java

        assertEquals(cursorInfo.markDelete.entryId, -1);

-        assertEquals(cursorInfo.individualDeletedMessages.size(), 1);
+        assertEquals(cursorInfo.individualDeletedMessages.size(), 2);


Is this due to a change in behavior?

Yes, ConcurrentOpenLongPairRangeSet has separate BitSet for every ledger and while creating ranges it doesn't merge range from different ledgers (doesn't merge last entry of previous ledger with first entry of next ledger) so, instead one combined ledger-range it returns multiple ranges when ledgers are different.

rdhabalia · 2019-05-21T05:26:43Z

rerun integration tests
rerun java8 tests

rdhabalia · 2019-05-21T05:32:18Z

@merlimat addressed your comments, added data-provider in existing ManagedCursor tests to test both usecases and also added separate test which test ack-holes and markDelete with both usecases.

rdhabalia · 2019-05-21T06:03:54Z

rerun cpp tests

rdhabalia · 2019-05-21T17:18:23Z

rerun cpp tests

### Motivation Some parameters are added in the `broker.conf` and `standalone.conf` files. However, those parameters are not updated in the docs. See the following PRs for details: #4150, #4066, #4197, #3819, #4261, #4273, #4320. ### Modifications Add those parameter info, and sync docs with the code. Does not update the description quite much, there are two reasons for this: 1. Keep doc content consistent with code. We need to update the description for those parameters in the code first, and then sync them in docs. 2. Will adopt a generator to generate those content automatically in the near future.

### Motivation Some parameters are added in the `broker.conf` and `standalone.conf` files. However, those parameters are not updated in the docs. See the following PRs for details: apache#4150, apache#4066, apache#4197, apache#3819, apache#4261, apache#4273, apache#4320. ### Modifications Add those parameter info, and sync docs with the code. Does not update the description quite much, there are two reasons for this: 1. Keep doc content consistent with code. We need to update the description for those parameters in the code first, and then sync them in docs. 2. Will adopt a generator to generate those content automatically in the near future.

rdhabalia added 3 commits March 12, 2019 17:16

[pulsar-common] add open Concurrent LongPair RangeSet

e6e8338

add open-range set methods

ca4c38b

[pulsar-broker] Cache unack-messageId into openRangeSet

f31a574

rdhabalia added the area/broker label Mar 14, 2019

rdhabalia added this to the 2.4.0 milestone Mar 14, 2019

rdhabalia self-assigned this Mar 14, 2019

rdhabalia requested review from jai1, massakam, merlimat, saandrews and sijie March 14, 2019 07:23

rdhabalia and others added 5 commits March 14, 2019 12:43

add forEach

51509e5

use forEach of the range

6c322df

add forEach with consumer

dd9e546

add Recyclable Position converter

41ae396

Merge branch 'master' into ml-range1

a390c01

rdhabalia mentioned this pull request May 17, 2019

[pulsar-common] add open Concurrent LongPair RangeSet #3818

Merged

merlimat added 2 commits May 18, 2019 07:45

Merge branch 'master' into ml-range1

e5340d8

Removed unused imports after merge

dfdfadd

merlimat reviewed May 18, 2019

View reviewed changes

address comments

cd1b18c

rdhabalia force-pushed the ml-range1 branch from 2f48784 to cd1b18c Compare May 21, 2019 05:30

merlimat approved these changes May 21, 2019

View reviewed changes

rdhabalia merged commit 5556f1b into apache:master May 21, 2019

This was referenced Jan 13, 2020

[docs] Update parameters info in the reference-configuration file streamnative/pulsar-archived#558

Closed

[docs] Update configuration information for 2.4.x releases #6047

Merged

massakam mentioned this pull request Jul 16, 2020

Messages that have already been acked are redelivered when upgrading Pulsar version #7554

Closed

rdhabalia mentioned this pull request Jan 24, 2021

[fix][broker] Support large number of unack message store for cursor recovery #9292

Merged

4 tasks

rdhabalia deleted the ml-range1 branch September 26, 2024 19:11

[pulsar-broker] Cache unack-messageId into openRangeSet #3819

[pulsar-broker] Cache unack-messageId into openRangeSet #3819

Uh oh!

Conversation

rdhabalia commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Result

Uh oh!

rdhabalia commented Apr 26, 2019

Uh oh!

rdhabalia commented May 10, 2019

Uh oh!

rdhabalia commented May 10, 2019

Uh oh!

rdhabalia commented May 13, 2019

Uh oh!

rdhabalia commented May 15, 2019

Uh oh!

codelipenghui commented May 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdhabalia commented May 21, 2019

Uh oh!

rdhabalia commented May 21, 2019

Uh oh!

rdhabalia commented May 21, 2019

Uh oh!

rdhabalia commented May 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rdhabalia commented Mar 14, 2019 •

edited

Loading