You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "coderzc (via GitHub)" <gi...@apache.org> on 2026/08/10 07:52:57 UTC
[PR] [fix][ml] Skip contiguous deleted ranges during reads [pulsar]
coderzc opened a new pull request, #26299:
URL: https://github.com/apache/pulsar/pull/26299
### Motivation
When an older unacknowledged entry pins a cursor's mark-delete position, acknowledgments for later entries can form a very large contiguous individually deleted range. This is common for Key_Shared subscriptions when consumers continue acknowledging messages after the blocking entry.
`ManagedLedgerImpl.internalReadFromLedger` pre-filters a scan window before reading it. When every position in the window is skipped, it currently advances only to the position after that window. Reading past a large individually deleted range therefore takes O(entries / batchSize) read-loop iterations instead of one range lookup.
For closed ledgers, each iteration also repeats the fully-acknowledged ledger check introduced in #24655. That check calls `getNumberOfEntries` and `RangeSetWrapper.cardinality`, which clones the cursor's per-ledger Roaring bitmap. Repeating it while walking a large acknowledged range can stall delivery and consume significant broker CPU.
### Modifications
- When a complete scan window is skipped, use `ManagedCursorImpl.getNextAvailablePosition` to hop to the end of the containing individually deleted range.
- Preserve the existing next-position behavior when no cursor is associated with the read or when the window was skipped for a reason other than individual acknowledgment.
- Add a regression test with a closed ledger, a pinned mark-delete position, and a 19,996-entry contiguous individually deleted range. The test verifies both the returned entries and that the reader examines only a bounded number of positions.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- `./gradlew :managed-ledger:test --tests org.apache.bookkeeper.mledger.impl.ManagedCursorSkipDeletedEntriesTest --no-daemon`
- `./gradlew :managed-ledger:test --tests 'org.apache.bookkeeper.mledger.impl.ManagedCursorTest.testReadEntriesWithSkipDeletedEntriesAndWithSkipConditions' --no-daemon`
- `./gradlew quickCheck --no-daemon`
The regression test examines 20,002 positions and fails on the unpatched implementation. With this change, the test passes by hopping over the deleted range.
### Does this pull request potentially affect one of the following parts:
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Skip contiguous deleted ranges during reads [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #26299:
URL: https://github.com/apache/pulsar/pull/26299
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Skip contiguous deleted ranges during reads [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #26299:
URL: https://github.com/apache/pulsar/pull/26299#discussion_r3750393626
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorSkipDeletedEntriesTest.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.mledger.impl;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Predicate;
+import lombok.Cleanup;
+import org.apache.bookkeeper.mledger.AsyncCallbacks.ReadEntriesCallback;
+import org.apache.bookkeeper.mledger.Entry;
+import org.apache.bookkeeper.mledger.ManagedLedgerConfig;
+import org.apache.bookkeeper.mledger.ManagedLedgerException;
+import org.apache.bookkeeper.mledger.Position;
+import org.apache.bookkeeper.mledger.PositionFactory;
+import org.apache.bookkeeper.test.MockedBookKeeperTestCase;
+import org.testng.annotations.Test;
+
+/**
+ * Regression test for reading over a cursor that has a very large contiguous range of individually
+ * deleted (acknowledged) entries while its mark-delete position is pinned by an older unacknowledged entry.
+ *
+ * <p>This is the shape a Key_Shared subscription ends up in when a single unacknowledged message blocks the
+ * mark-delete position while consumers keep acknowledging everything after it. Reads then have to get
+ * past tens of millions of already-acknowledged entries before they can fill a batch.
+ *
+ * <p>The skip pre-filter in {@link ManagedLedgerImpl#internalReadFromLedger} used to advance the read
+ * position by the size of the scan window when every entry in that window was already acknowledged, instead of
+ * hopping over the whole deleted range via
+ * {@link ManagedCursorImpl#getNextAvailablePosition(Position)}. That turned a single range hop into
+ * O(entries / batchSize) read-loop iterations. Each iteration re-ran
+ * {@code isLedgerFullyAcked} -> {@code ManagedCursorImpl#getNumberOfEntries} ->
+ * {@code RangeSetWrapper#cardinality}, which clones the cursor's per-ledger Roaring bitmap.
Review Comment:
Minor: `RangeSetWrapper` no longer exists on master, and the current `rank()` path does not clone the bitmap. Could we update this comment and the PR description?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Skip contiguous deleted ranges during reads [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #26299:
URL: https://github.com/apache/pulsar/pull/26299#discussion_r3750393626
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorSkipDeletedEntriesTest.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.mledger.impl;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Predicate;
+import lombok.Cleanup;
+import org.apache.bookkeeper.mledger.AsyncCallbacks.ReadEntriesCallback;
+import org.apache.bookkeeper.mledger.Entry;
+import org.apache.bookkeeper.mledger.ManagedLedgerConfig;
+import org.apache.bookkeeper.mledger.ManagedLedgerException;
+import org.apache.bookkeeper.mledger.Position;
+import org.apache.bookkeeper.mledger.PositionFactory;
+import org.apache.bookkeeper.test.MockedBookKeeperTestCase;
+import org.testng.annotations.Test;
+
+/**
+ * Regression test for reading over a cursor that has a very large contiguous range of individually
+ * deleted (acknowledged) entries while its mark-delete position is pinned by an older unacknowledged entry.
+ *
+ * <p>This is the shape a Key_Shared subscription ends up in when a single unacknowledged message blocks the
+ * mark-delete position while consumers keep acknowledging everything after it. Reads then have to get
+ * past tens of millions of already-acknowledged entries before they can fill a batch.
+ *
+ * <p>The skip pre-filter in {@link ManagedLedgerImpl#internalReadFromLedger} used to advance the read
+ * position by the size of the scan window when every entry in that window was already acknowledged, instead of
+ * hopping over the whole deleted range via
+ * {@link ManagedCursorImpl#getNextAvailablePosition(Position)}. That turned a single range hop into
+ * O(entries / batchSize) read-loop iterations. Each iteration re-ran
+ * {@code isLedgerFullyAcked} -> {@code ManagedCursorImpl#getNumberOfEntries} ->
+ * {@code RangeSetWrapper#cardinality}, which clones the cursor's per-ledger Roaring bitmap.
Review Comment:
Minor: `RangeSetWrapper` no longer exists on master, and the current `rank()` path does not clone the bitmap. Could we update this comment and the PR description?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org