[Enhancement] Nested read-write lock issue in ManagedCursor

[Denovo1998]

Search before asking

• I searched in the issues and found nothing similar.

Motivation

The following are all related to the org.apache.bookkeeper.mledger.impl.ManagedCursorImpl#isMessageDeleted:

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

Lines 3542 to 3551 in 89ccb73

	@Override
	public boolean isMessageDeleted(Position position) {
	lock.readLock().lock();
	try {
	return position.compareTo(markDeletePosition) <= 0
	|| individualDeletedMessages.contains(position.getLedgerId(), position.getEntryId());
	} finally {
	lock.readLock().unlock();
	}
	}

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.
   

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

   Lines 2328 to 2348 in 89ccb73

   	lock.writeLock().lock();
   	try {
   	if (log.isDebugEnabled()) {
   	log.debug("[{}] [{}] Deleting individual messages at {}. Current status: {} - md-position: {}",
   	ledger.getName(), name, positions, individualDeletedMessages, markDeletePosition);
   	}
   	for (Position pos : positions) {
   	Position position = requireNonNull(pos);
   	if (ledger.getLastConfirmedEntry().compareTo(position) < 0) {
   	if (log.isDebugEnabled()) {
   	log.debug(
   	"[{}] Failed mark delete due to invalid markDelete {} is ahead of last-confirmed-entry {} "
   	+ "for cursor [{}]", ledger.getName(), position, ledger.getLastConfirmedEntry(), name);
   	}
   	callback.deleteFailed(new ManagedLedgerException("Invalid mark deleted position"), ctx);
   	return;
   	}
   	if (isMessageDeleted(position)) {

2. The performance impact of nested read locks can be negligible, but frequent acquisition and release of locks will slightly increase system overhead.
   

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

   Lines 1570 to 1575 in 89ccb73

   	lock.readLock().lock();
   	try {
   	positions.stream().filter(this::isMessageDeleted).forEach(alreadyAcknowledgedPositions::add);
   	} finally {
   	lock.readLock().unlock();
   	}

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?

• I'm willing to submit a PR!

[Denovo1998]

@dao-jun @lhotari PTAL.

[dao-jun]

Personally, I don't think we need to fix it, Read/Write Lock is ReentrantLock, and it just compare the lock owner of the AQS and the current thread when we required a ReadLock after required a WriteLock.

[lhotari]

2. The performance impact of nested read locks can be negligible, but frequent acquisition and release of locks will slightly increase system overhead.

Makes sense. I have a large changeset to address multiple performance issues with Broker cache and one of the changes is related: lhotari@a0d0b54 . I haven't yet split this large changeset into PRs for review.

[Denovo1998]

@lhotari Oh, I saw it! If you think it‘s necessary. Maybe you can solve the read-write lock nesting problem mentioned in this issue in your pr.

[lhotari]

@lhotari Oh, I saw it! If you think it‘s necessary. Maybe you can solve the read-write lock nesting problem mentioned in this issue in your pr.

@Denovo1998 you can go ahead and create a PR if you wish.

[lhotari]

Addressed by #23609