[fix][broker] patch #5809: Fix the ledgerID not found cause NPE. #15098

horizonzy · 2022-04-10T05:52:30Z

Master Issue: #5669 #5809

Motivation

Relational code:

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

Lines 48 to 63 in af35164

    
           public static OpReadEntry create(ManagedCursorImpl cursor, PositionImpl readPositionRef, int count, 
        
                   ReadEntriesCallback callback, Object ctx, PositionImpl maxPosition) { 
        
               OpReadEntry op = RECYCLER.get(); 
        
               op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op); 
        
               op.cursor = cursor; 
        
               op.count = count; 
        
               op.callback = callback; 
        
               op.entries = Lists.newArrayList(); 
        
               if (maxPosition == null) { 
        
                   maxPosition = PositionImpl.LATEST; 
        
               } 
        
               op.maxPosition = maxPosition; 
        
               op.ctx = ctx; 
        
               op.nextReadPosition = PositionImpl.get(op.readPosition); 
        
               return op; 
        
           }

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

Lines 2230 to 2245 in af35164

    
           PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) { 
        
               Long ledgerId = ledgers.ceilingKey(position.getLedgerId()); 
        
               if (null == ledgerId) { 
        
                   opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("The ceilingKey(K key" 
        
                           + ") method is used to return the least key greater than or equal to the given key, " 
        
                           + "or null if there is no such key"), null); 
        
               } 
        
               if (ledgerId != position.getLedgerId()) { 
        
                   // The ledger pointed by this position does not exist anymore. It was deleted because it was empty. We need 
        
                   // to skip on the next available ledger 
        
                   position = new PositionImpl(ledgerId, 0); 
        
               } 
        
               return position; 
        
           }

In #5809, there be 3 potential problem.

case 1: In OpAddEntry construction, it use ManagedLedgerImpl#startReadOperationOnLedger to setup readPosition, in startReadOperationOnLedger, it will callback OpAddEntry, but now some property not initial yet(like curtos,callback, entries...), cause npe.
case 2:
in startReadOperationOnLedger, the callback did't pass on ctx, the callback process need it, also case npe.
case 3:
in startReadOperationOnLedger, it didn't return when not found ledger id. The judgement unbox also case npe.

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

Line 2238 in af35164

if (ledgerId != position.getLedgerId()) {

Modifications

patch #5809

Documentation

no-need-doc

github-actions · 2022-04-10T05:52:50Z

@horizonzy: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-04-10T05:52:56Z

@horizonzy:Thanks for providing doc info!

mattisonchao

Overall LGTM, but i have a question:
Could we check for an OpReadEntry earlier (to avoid creating useless objects) or throw an exception (to achieve fail fast) when creating it?
Please let me know what you think, thanks~

horizonzy · 2022-04-10T07:22:05Z

Overall LGTM, but i have a question: Could we check for an OpReadEntry earlier (to avoid creating useless objects) or throw an exception (to achieve fail fast) when creating it? Please let me know what you think, thanks~

yep, I have thought it, but it's not atomic, it need user make invalid outside. This way more secure.

horizonzy · 2022-04-10T07:28:21Z

Overall LGTM, but i have a question: Could we check for an OpReadEntry earlier (to avoid creating useless objects) or throw an exception (to achieve fail fast) when creating it? Please let me know what you think, thanks~

And the operation startReadOperationOnLedger not only happen at opAddEntry construction, so I change the invalid in the method.

mattisonchao · 2022-04-10T07:33:26Z

Overall LGTM, but i have a question: Could we check for an OpReadEntry earlier (to avoid creating useless objects) or throw an exception (to achieve fail fast) when creating it? Please let me know what you think, thanks~

yep, I have thought it, but it's not atomic, it need user make invalid outside. This way more secure.

Got it. I was just wondering if there is a race condition here.
I would also like to say that we can throw an exception when we get an empty position. But it doesn't look particularly different from your approach.

mattisonchao

Great work! LGTM +1

Jason918 · 2022-04-10T09:05:32Z

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

-            opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("The ceilingKey(K key"
-                    + ") method is used to return the least key greater than or equal to the given key, "
-                    + "or null if there is no such key"), null);
+            opReadEntry.makeInvalid();


This changes the behavior of the method. So we need to go through all the usage of this method.
As far as I can see,

This line makes no sense any more.

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

Line 142 in a242f03

cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);

Also affects the OpReadEntry.create in

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

Line 765 in 5cf3fa0

OpReadEntry op = OpReadEntry.create(this, readPosition, numberOfEntriesToRead, callback,

Thanks for your reminder. I have checked it, found another problem, I will fix it at another pr.
This one patch just for npe.

location 1:
It's useless code, I remove it at #15104. So needn't check it.

location 2:
It's a wait opReadEntry, when opAddEntry completed, it will nofity wait opReadEntry read again. The logicment is not conflict with this changes. And I fix the bad behavior when wait opReadEntry read at #15102

It's a wait opReadEntry, when opAddEntry completed, it will nofity wait opReadEntry read again.

Can you add a unit test to cover this path?
It seems readPosition of this invalid OpReadEntry is used before asyncReadEntries in hasMoreEntries

Can you add a unit test to cover this path? It seems readPosition of this invalid OpReadEntry is used before asyncReadEntries in hasMoreEntries

Maybe not, it will still be used in asyncReadEntries.

At line_768, the wait OpReadEntry set to property WAITING_READ_OP_UPDATER.

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

Line 768 in c163158

if (!WAITING_READ_OP_UPDATER.compareAndSet(this, null, op)) {

In notifyEntriesAvailable, get wait OpReadEntry from WAITING_READ_OP_UPDATER, and handle it to asyncReadEntries .

And in notifyEntriesAvailable, there be a bad behavior. fixes at #15102

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

Lines 2749 to 2772 in c163158

void notifyEntriesAvailable() {

if (log.isDebugEnabled()) {

log.debug("[{}] [{}] Received ml notification", ledger.getName(), name);

}

OpReadEntry opReadEntry = WAITING_READ_OP_UPDATER.getAndSet(this, null);

if (opReadEntry != null) {

if (log.isDebugEnabled()) {

log.debug("[{}] [{}] Received notification of new messages persisted, reading at {} -- last: {}",

ledger.getName(), name, opReadEntry.readPosition, ledger.lastConfirmedEntry);

log.debug("[{}] Consumer {} cursor notification: other counters: consumed {} mdPos {} rdPos {}",

ledger.getName(), name, messagesConsumedCounter, markDeletePosition, readPosition);

}

PENDING_READ_OPS_UPDATER.incrementAndGet(this);

opReadEntry.readPosition = (PositionImpl) getReadPosition();

ledger.asyncReadEntries(opReadEntry);

} else {

// No one is waiting to be notified. Ignore

if (log.isDebugEnabled()) {

log.debug("[{}] [{}] Received notification but had no pending read operation", ledger.getName(), name);

}

}

}

Add unit to cover wait opReadEntry case. And close #15102, the handle push at this pr.

all done, pls check it again when you convenient.

…ld re check.

lhotari · 2022-04-10T11:31:40Z

There's also #12396 which fixes a NPE in OpReadEntry. That might be another problem that it's fixing.

horizonzy · 2022-04-10T12:12:55Z

There's also #12396 which fixes a NPE in OpReadEntry. That might be another problem that it's fixing.

well, I will check it together.

horizonzy · 2022-04-10T12:23:12Z

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

+            return null;
+        } else {
+            // for wait opReadEntry, the readPosition will recalculate.
+            opReadEntry.makeValid();


Here for wait opReadEntry, it maybe change from invalid to valid. So add this logicment.

github-actions · 2022-05-26T02:23:29Z

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

github-actions · 2022-06-26T02:15:09Z

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

patch apache#5809: Fix the ledgerID not found cause NPE.

4956976

github-actions bot assigned horizonzy Apr 10, 2022

github-actions bot added the doc-label-missing label Apr 10, 2022

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Apr 10, 2022

horizonzy added 2 commits April 10, 2022 14:47

code style fix.

f7c52fa

error msg tweak.

f9f0667

mattisonchao reviewed Apr 10, 2022

View reviewed changes

mattisonchao approved these changes Apr 10, 2022

View reviewed changes

Jason918 requested changes Apr 10, 2022

View reviewed changes

for wait opReadEntry, the readPosition will recalculate, invalid shou…

8da51bb

…ld re check.

horizonzy commented Apr 10, 2022

View reviewed changes

dave2wave requested review from lhotari and michaeljmarshall April 10, 2022 20:48

add test for wait OpReadEntry.

23b3dd7

This was referenced Apr 11, 2022

[fix] [broker] When notify wait opReadEntry, use same policy to calculate readPosition. #15102

Closed

When notify wait opReadEntry, the readPosition calculated policy is different from opReadEntry.create() #15101

Closed

add unit for notify wait opReadEntry still invalid situation.

4ab367e

horizonzy mentioned this pull request Apr 11, 2022

Fix NPE when create OpEntry #12396

Closed

codelipenghui added this to the 2.11.0 milestone Apr 25, 2022

github-actions bot added the lifecycle/stale label May 26, 2022

horizonzy closed this Jun 30, 2022

	public static OpReadEntry create(ManagedCursorImpl cursor, PositionImpl readPositionRef, int count,
	ReadEntriesCallback callback, Object ctx, PositionImpl maxPosition) {
	OpReadEntry op = RECYCLER.get();
	op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op);
	op.cursor = cursor;
	op.count = count;
	op.callback = callback;
	op.entries = Lists.newArrayList();
	if (maxPosition == null) {
	maxPosition = PositionImpl.LATEST;
	}
	op.maxPosition = maxPosition;
	op.ctx = ctx;
	op.nextReadPosition = PositionImpl.get(op.readPosition);
	return op;
	}

	PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
	Long ledgerId = ledgers.ceilingKey(position.getLedgerId());
	if (null == ledgerId) {
	opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("The ceilingKey(K key"
	+ ") method is used to return the least key greater than or equal to the given key, "
	+ "or null if there is no such key"), null);
	}

	if (ledgerId != position.getLedgerId()) {
	// The ledger pointed by this position does not exist anymore. It was deleted because it was empty. We need
	// to skip on the next available ledger
	position = new PositionImpl(ledgerId, 0);
	}

	return position;
	}

	void notifyEntriesAvailable() {
	if (log.isDebugEnabled()) {
	log.debug("[{}] [{}] Received ml notification", ledger.getName(), name);
	}
	OpReadEntry opReadEntry = WAITING_READ_OP_UPDATER.getAndSet(this, null);

	if (opReadEntry != null) {
	if (log.isDebugEnabled()) {
	log.debug("[{}] [{}] Received notification of new messages persisted, reading at {} -- last: {}",
	ledger.getName(), name, opReadEntry.readPosition, ledger.lastConfirmedEntry);
	log.debug("[{}] Consumer {} cursor notification: other counters: consumed {} mdPos {} rdPos {}",
	ledger.getName(), name, messagesConsumedCounter, markDeletePosition, readPosition);
	}

	PENDING_READ_OPS_UPDATER.incrementAndGet(this);
	opReadEntry.readPosition = (PositionImpl) getReadPosition();
	ledger.asyncReadEntries(opReadEntry);
	} else {
	// No one is waiting to be notified. Ignore
	if (log.isDebugEnabled()) {
	log.debug("[{}] [{}] Received notification but had no pending read operation", ledger.getName(), name);
	}
	}
	}

[fix][broker] patch #5809: Fix the ledgerID not found cause NPE. #15098

[fix][broker] patch #5809: Fix the ledgerID not found cause NPE. #15098

Uh oh!

Conversation

horizonzy commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Uh oh!

github-actions bot commented Apr 10, 2022

Uh oh!

github-actions bot commented Apr 10, 2022

Uh oh!

mattisonchao left a comment

Choose a reason for hiding this comment

Uh oh!

horizonzy commented Apr 10, 2022

Uh oh!

horizonzy commented Apr 10, 2022

Uh oh!

mattisonchao commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattisonchao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jason918 Apr 10, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Apr 10, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jason918 Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari commented Apr 10, 2022

Uh oh!

horizonzy commented Apr 10, 2022

Uh oh!

horizonzy Apr 10, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 26, 2022

Uh oh!

github-actions bot commented Jun 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

horizonzy commented Apr 10, 2022 •

edited

Loading

mattisonchao commented Apr 10, 2022 •

edited

Loading

mattisonchao left a comment •

edited

Loading

horizonzy Apr 10, 2022 •

edited

Loading