PIP-174: New managed ledger entry cache implementation #15955

merlimat · 2022-06-07T00:48:00Z

Motivation

PIP-174: #15954

Provide new SharedEntryCacheManagerImpl implementation

doc-complete

github-actions · 2022-06-07T00:48:24Z

@merlimat:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

github-actions · 2022-06-07T00:48:24Z

@merlimat:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

asafm · 2022-06-19T17:45:55Z

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

+            int offset = (int) (value >> 32);
+            int entryLen = (int) value;
+
+            ByteBuf entry = PulsarByteBufAllocator.DEFAULT.buffer(entryLen, entryLen);


Since it's draft PR, I'm writing here an idea that crossed my mind.
On each get, we pay the penalty of creating ByteBuf, both heap object and direct memory allocation, then copying.
If would return a ByteBuf which is a linked ByteBuf (view) to the original ByteBuf?
It's still valid as long as we don't call clear().
Perhaps we can maintain an ever-increasing version number, which upon clear we increase it.
We can return a CachedByteBuf, which has a link to the cache and version it was cut from. It version got bigger, it means it's invalidated and can't be used anymore.
CachedByteBuf also be pooled if needed, since they are just long and a ByteBuf.
Just an idea

Yes, we could return a "retained slice" (a ByteBuf that increments the ref-count and points to a portion of the original buffer) and avoid the copy on the read path.
The problem would be that this buffer could stay alive for an indefinite amount of time, in the case of some consumer connections being slow. With this, we'd be retaining a whole 1GB buffer even if a small message is pending on a TCP connection, and we cannot just overwrite the old cache segment when rotating because the reader could still be there.

Since we already have a flag to control the copy/not-copy of the cache, another approach I was thinking of was to keep maps of the original ByteBuf (so that we also eliminate the copy on insertion in the cache).
We still do the rotation based on rotating the segments, where each segment has its own hash map.

Since we already have a flag to control the copy/not-copy of the cache, another approach I was thinking of was to keep maps of the original ByteBuf (so that we also eliminate the copy on insertion in the cache).
We still do the rotation based on rotating the segments, where each segment has its own hash map.

I didn't understand that part. What do you mean by maps of original bytebuf?

Yes, just incrementing the ref-count. It's similar to what we are currently doing, though without the overly-complex logic for cache eviction.

github-actions · 2022-07-23T02:14:46Z

The pr had no activity for 30 days, mark with Stale label.

codelipenghui

LGTM, just left some minor comments.

codelipenghui · 2022-07-28T03:54:58Z

...dger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/SharedEntryCacheManagerImpl.java

+                    } else {
+                        break;
+                    }


Is it possible that the cache of subsequent entries still in this segment? after we get null from this segment, we will move to the next segment.

Is it possible that the cache of subsequent entries still in this segment? after we get null from this segment, we will move to the next segment.

+1

codelipenghui · 2022-07-28T04:05:31Z

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

+        if (cachedEntries.size() == entriesToRead) {
+            // All entries found in cache
+            entryCacheManager.getFactoryMBean().recordCacheHits(entriesToRead, totalCachedSize);
+            if (log.isDebugEnabled()) {
+                log.debug("[{}] Ledger {} -- Found in cache entries: {}-{}", ml.getName(), ledgerId, firstEntry,
+                        lastEntry);
+            }
+
+            callback.readEntriesComplete(cachedEntries, ctx);
+
+        } else {
+            if (!cachedEntries.isEmpty()) {
+                cachedEntries.forEach(entry -> entry.release());
+            }


Looks like we are safe to return part of the data from the cache? I'm not sure if I missed something, a little waste of resources to skip a partially hit cache data. The old implementation also follows this way, so we can also use a separate PR to improve this part if possible.

codelipenghui · 2022-07-28T04:09:57Z

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

+
+    @Override
+    public long getSize() {
+        return 0;


It's better to add some comments here, return 0 here to avoid the cache eviction, and will not expose topic-level cache size metrics since the implementation shared the cache across all topics.

Technoboy- · 2022-08-03T11:36:32Z

Checkstyle failed @merlimat

mattisonchao · 2022-08-04T07:33:20Z

The test testPulsarSinkDLQ is flaky. It work fine at my local env. So, we can try re-run broker group2

#16578

mattisonchao · 2022-08-04T08:16:43Z

/pulsarbot run-failure-checks

Jason918 · 2022-08-04T09:20:52Z

...n/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java

@@ -204,6 +204,12 @@ public LongPair get(long key1, long key2) {
        return getSection(h).get(key1, key2, (int) h);
    }

+    public long getFirstValue(long key1, long key2) {


Better to add some doc here. The method name is a bit confusing.

Suggested change

public long getFirstValue(long key1, long key2) {

/**

* @return get(key1, key2).first;

*/

public long getFirstValue(long key1, long key2) {

Jason918 · 2022-08-04T09:39:10Z

...dger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/SharedEntryCacheManagerImpl.java

+
+    @Override
+    public void updateCacheSizeAndThreshold(long maxSize) {
+


This method should be supported when user update managedLedgerCacheSizeMB. We can add an error log here to let user know this.

Jason918 · 2022-08-04T09:46:58Z

...dger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/SharedEntryCacheManagerImpl.java

+                    } else {
+                        break;
+                    }


Is it possible that the cache of subsequent entries still in this segment? after we get null from this segment, we will move to the next segment.

+1

...src/main/java/org/apache/bookkeeper/mledger/impl/cache/SharedCacheSegmentBufferRefCount.java

...ger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/SharedCacheSegmentBufferCopy.java

eolivelli

LGTM

I would commit this to 2.11 only in case of keeping the old cache strategy as default, because nobody had enough time to test this new cache implementation and it is very dangerous to make it the default implementation now that we are close to the release.

eolivelli · 2022-08-18T11:41:46Z

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

 import org.testng.annotations.Test;

 public class EntryCacheManagerTest extends MockedBookKeeperTestCase {

    ManagedLedgerImpl ml1;
    ManagedLedgerImpl ml2;

+    @DataProvider(name = "EntryCacheManagerClass")
+    public static Object[][] primeNumbers() {


nit: primeNumbers ?

github-actions · 2022-09-18T02:20:46Z

The pr had no activity for 30 days, mark with Stale label.

tisonkun · 2022-12-09T14:46:00Z

@merlimat it seems the review has been already done but we conflict a few files here.

Could you rebase the patch onto master so that we can proceed the PR?

github-actions · 2023-01-10T02:05:15Z

The pr had no activity for 30 days, mark with Stale label.

lhotari

Please rebase and adapt PendingReadsManager so that it can be used in SharedEntryCacheImpl besides RangeEntryCacheImpl. PendingReadsManager was introduced by @eolivelli in PR #17241 and it resulted in huge improvements.

lhotari · 2024-04-05T07:00:48Z

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

+    public void asyncReadEntry(ReadHandle lh, long firstEntry, long lastEntry, boolean isSlowestReader,
+                               AsyncCallbacks.ReadEntriesCallback callback, Object ctx) {
+        final long ledgerId = lh.getId();
+        final int entriesToRead = (int) (lastEntry - firstEntry) + 1;
+
+        if (log.isDebugEnabled()) {
+            log.debug("[{}] Reading entries range ledger {}: {} to {}", ml.getName(), ledgerId, firstEntry, lastEntry);
+        }
+
+        List<Entry> cachedEntries = new ArrayList<>(entriesToRead);
+        long totalCachedSize = entryCacheManager.getRange(ledgerId, firstEntry, lastEntry, cachedEntries);
+
+        if (cachedEntries.size() == entriesToRead) {
+            final List<Entry> entriesToReturn = Lists.newArrayListWithExpectedSize(entriesToRead);
+            // All entries found in cache
+            for (Entry entry : cachedEntries) {
+                entriesToReturn.add(EntryImpl.create((EntryImpl) entry));
+                entry.release();
+            }
+            // All entries found in cache
+            entryCacheManager.getFactoryMBean().recordCacheHits(entriesToReturn.size(), totalCachedSize);
+            if (log.isDebugEnabled()) {
+                log.debug("[{}] Ledger {} -- Found in cache entries: {}-{}", ml.getName(), ledgerId, firstEntry,
+                        lastEntry);
+            }
+            callback.readEntriesComplete(entriesToReturn, ctx);
+
+        } else {
+            if (!cachedEntries.isEmpty()) {
+                cachedEntries.forEach(entry -> entry.release());
+            }
+
+            // Read all the entries from bookkeeper
+            lh.readAsync(firstEntry, lastEntry).thenAcceptAsync(
+                    ledgerEntries -> {
+                        requireNonNull(ml.getName());
+                        requireNonNull(ml.getExecutor());
+


I guess PendingReadsManager should be adapted and used here? It was introduced by #17241 and resulted in huge improvements.

merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Jun 7, 2022

merlimat added this to the 2.11.0 milestone Jun 7, 2022

merlimat self-assigned this Jun 7, 2022

github-actions bot added the doc-label-missing label Jun 7, 2022

merlimat added doc-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels Jun 7, 2022

lhotari mentioned this pull request Jun 7, 2022

[Performance] Optimize allocations in ManagedLedgerImpl.getEarlierReadPositionForActiveCursors()/ManagedCursorContainer.iterator() #9958

Closed

asafm reviewed Jun 19, 2022

View reviewed changes

github-actions bot added the Stale label Jul 23, 2022

PIP-174: New managed ledger entry cache implementation

5544ed2

merlimat force-pushed the new-entry-cache branch from ea6f491 to 5544ed2 Compare July 25, 2022 02:32

merlimat marked this pull request as ready for review July 25, 2022 02:33

merlimat added doc-complete Your PR changes impact docs and the related docs have been already added. release/note-required and removed doc-required Your PR changes impact docs and you will update later. labels Jul 25, 2022

codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022

codelipenghui approved these changes Jul 28, 2022

View reviewed changes

Technoboy- removed the Stale label Aug 3, 2022

Technoboy- and others added 2 commits August 3, 2022 15:46

Merge branch 'master' into new-entry-cache

8f31f4c

Fixed test

519cb01

Technoboy- and others added 3 commits August 3, 2022 20:26

fix checkstyle.

2273fc6

Removed unused import

c2a27e9

Fix wrong configuration

46d4547

Fix read entires

2cc107d

mattisonchao approved these changes Aug 4, 2022

View reviewed changes

Jason918 reviewed Aug 4, 2022

View reviewed changes

Technoboy- force-pushed the new-entry-cache branch from d620513 to 2cc107d Compare August 16, 2022 13:39

Technoboy- added 3 commits August 16, 2022 21:40

Merge branch 'master' into new-entry-cache

9945374

fix comment.

870d15a

fix modernizer plugin check.

f6855e4

eolivelli approved these changes Aug 18, 2022

View reviewed changes

github-actions bot added the Stale label Sep 18, 2022

github-actions bot removed the Stale label Dec 10, 2022

github-actions bot added the Stale label Jan 10, 2023

RobertIndie modified the milestones: 2.11.0, 3.1.0 Apr 11, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

codelipenghui added release/important-notice The changes which are important should be mentioned in the release note and removed release/note-required labels Aug 22, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

lhotari requested changes Apr 5, 2024

View reviewed changes

lhotari mentioned this pull request May 8, 2024

We need a size limit for cache of single managedLeger #22621

Open

2 tasks

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-174: New managed ledger entry cache implementation #15955

PIP-174: New managed ledger entry cache implementation #15955

merlimat commented Jun 7, 2022 •

edited by github-actions bot

Loading

github-actions bot commented Jun 7, 2022

github-actions bot commented Jun 7, 2022

asafm Jun 19, 2022

merlimat Jun 22, 2022

asafm Jun 22, 2022 •

edited

Loading

merlimat Jun 22, 2022

github-actions bot commented Jul 23, 2022

codelipenghui left a comment

codelipenghui Jul 28, 2022

Jason918 Aug 4, 2022

codelipenghui Jul 28, 2022

codelipenghui Jul 28, 2022

Technoboy- commented Aug 3, 2022

mattisonchao commented Aug 4, 2022 •

edited

Loading

mattisonchao commented Aug 4, 2022

Jason918 Aug 4, 2022

Jason918 Aug 4, 2022

Jason918 Aug 4, 2022

eolivelli left a comment

eolivelli Aug 18, 2022

github-actions bot commented Sep 18, 2022

tisonkun commented Dec 9, 2022

github-actions bot commented Jan 10, 2023

lhotari left a comment

lhotari Apr 5, 2024


		@Override
		public void updateCacheSizeAndThreshold(long maxSize) {

PIP-174: New managed ledger entry cache implementation #15955

Are you sure you want to change the base?

PIP-174: New managed ledger entry cache implementation #15955

Conversation

merlimat commented Jun 7, 2022 • edited by github-actions bot Loading

Motivation

github-actions bot commented Jun 7, 2022

github-actions bot commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asafm Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 23, 2022

codelipenghui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Technoboy- commented Aug 3, 2022

mattisonchao commented Aug 4, 2022 • edited Loading

mattisonchao commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2022

tisonkun commented Dec 9, 2022

github-actions bot commented Jan 10, 2023

lhotari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Jun 7, 2022 •

edited by github-actions bot

Loading

asafm Jun 22, 2022 •

edited

Loading

mattisonchao commented Aug 4, 2022 •

edited

Loading