apache / pulsar

Apache Pulsar - distributed pub-sub messaging system
https://pulsar.apache.org/
Apache License 2.0
14.25k stars 3.58k forks source link

[Enhancement] Nested read-write lock issue in ManagedCursor #23605

Open Denovo1998 opened 2 hours ago

Denovo1998 commented 2 hours ago

Search before asking

Motivation

The following are all related to the org.apache.bookkeeper.mledger.impl.ManagedCursorImpl#isMessageDeleted: https://github.com/apache/pulsar/blob/89ccb7361b80dfb7897ea2555dfb6e884f62b916/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L3542-L3551

  1. A write lock has already been acquired, but a read lock has been acquired as well. This doesn't pose a major problem, but it does result in some unnecessary additional overhead. https://github.com/apache/pulsar/blob/89ccb7361b80dfb7897ea2555dfb6e884f62b916/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L2328-L2348

  2. The performance impact of nested read locks can be negligible, but frequent acquisition and release of locks will slightly increase system overhead. https://github.com/apache/pulsar/blob/89ccb7361b80dfb7897ea2555dfb6e884f62b916/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1570-L1575

Solution

Add a method that does not require a read lock.

    private boolean isMessageDeletedWithOutReadLock(Position position) {
        return position.compareTo(markDeletePosition) <= 0
                || individualDeletedMessages.contains(position.getLedgerId(), position.getEntryId());
    }

Alternatives

No response

Anything else?

I think the modification at 22246 here is for code reuse, but the nested read-write locks in these two segments may need to be modified.

If such a modification is necessary, please let me know, and I will submit a simple PR.

Are you willing to submit a PR?

Denovo1998 commented 2 hours ago

@dao-jun @lhotari PTAL.