[fix][ML] Fix offload read handle NPE. by horizonzy · Pull Request #17056 · apache/pulsar

horizonzy · 2022-08-11T03:09:43Z

Motivation

This issue was introduced by #11389

Now, when we get offload ReadHandle(BlobStoreBackedReadHandleImpl) and then use it to read data (Non-durable cursor read). Maybe the offload ReadHandle we get already is invalidated.

There is a race condition, maybe after we get the ReadHandle, before use it to read.
The method ManagedLedgerImpl#internalTrimLedgers invalidate this ReadHandle (Cause in this method, it just calculate by durable-cursor, but there maybe a non-durable cursor read data) so we use the invalidated offload read handle to read data, invalidated will close the read handle.

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

Line 2542 in 96930fd

invalidateReadHandle(ls.getLedgerId());

In the close method, it will set index.segmentMetadata=null, so when getLastAddConfirmed, case NPE.

pulsar/tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImpl.java

Line 100 in bc68f02

index.close();

The NPE stack info

rg.apache.bookkeeper.mledger.ManagedLedgerException: java.lang.NullPointerException
Caused by: java.lang.NullPointerException
	at org.apache.bookkeeper.mledger.offload.jcloud.impl.BlobStoreBackedReadHandleImpl.getLastAddConfirmed(BlobStoreBackedReadHandleImpl.java:200) ~[?:?]
	at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.internalReadFromLedger(ManagedLedgerImpl.java:1870) ~[io.streamnative-managed-ledger-2.9.2.23.jar:2.9.2.23]
	at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.lambda$asyncReadEntries$17(ManagedLedgerImpl.java:1719) ~[io.streamnative-managed-ledger-2.9.2.23.jar:2.9.2.23]
	at java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:714) ~[?:?]
	at java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:506) ~[?:?]
	at java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2073) ~[?:?]
	at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.lambda$null$20(ManagedLedgerImpl.java:1790) ~[io.streamnative-managed-ledger-2.9.2.23.jar:2.9.2.23]
	at java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:859) [?:?]
	at java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:837) [?:?]
	at java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:478) [?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [io.netty-netty-common-4.1.77.Final.jar:4.1.77.Final]
	at java.lang.Thread.run(Thread.java:829) [?:?]
03:30:51.930 [broker-topic-workers-OrderedExecutor-3-0] ERROR org.apache.pulsar.broker.service.persistent.PersistentDispatcherSingleActiveConsumer - [persistent://public/default/test / reader-51d0ba5f20-Consumer{subscription=PersistentSubscription{topic=persistent://public/default/test, name=reader-51d0ba5f20}, consumerId=0, consumerName=reader-0, address=/10.244.1.143:51782}] Error reading entries at 0:101 : java.lang.NullPointerException - Retrying to read in 15.0 seconds

Modifications

1.Fix the NPE.
2.Check the invalidate ledgerId is less than the slowest non-durable, reduce the npe probability.

Documentation

Check the box below or label this PR directly.

Need to update docs?

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

hangc0276

Nice catch! LGTM.

hangc0276 · 2022-08-11T03:25:24Z

            }
        });
+        if (slowestNonDurableReadPosition.get() != null) {
+            ManagedLedger managedLedger = getManagedLedger();


You can directly use ledger.updateTheSlowestNonDurableReadPosition

Agrre, and there will reset ManagedLedgerImpl.slowestNonDurableReadPosition when there is no non-duranble cursor, fixed it in next commit.

codelipenghui · 2022-08-11T04:05:37Z

            }
-        } else if (exception.getCause() instanceof TransactionBufferException.TransactionNotSealedException) {
+        } else if (exception.getCause() instanceof TransactionBufferException.TransactionNotSealedException
+                || exception.getCause() instanceof ManagedLedgerException.OffloadReadHandleClosedException) {


Looks like we don't want to handle the race condition of trimming ledgers and reading data, instead, to re-trigger the read operation?

Yes, it's difficult to handle the race condition, the offload read handle execute read operation in offload-executor, before the read real execute, the read handle maybe invalidated.

codelipenghui · 2022-08-11T04:07:56Z

And can we add a test to make sure the reader handle will not invalidate if we the slowest read position haven't go through it yet.

horizonzy · 2022-08-12T10:09:15Z

Please review this pr again, use the new way to get the slowest non-durable read position.

michaeljmarshall · 2022-08-16T04:38:19Z

Heads up, cherry picking this commit to older branches broke them because of a missing import. I fixed 2.11 here: 74390d4

(cherry picked from commit 6fc48d1)

Jason918 · 2022-09-04T13:02:54Z

@horizonzy Can you help open a new PR to branch-2.10? There are a lot conflict when cherry-pick directly.

horizonzy · 2022-09-04T13:05:10Z

alright.

This pr cherry-pick from apache#17056.

horizonzy · 2022-09-05T15:40:55Z

@horizonzy Can you help open a new PR to branch-2.10? There are a lot conflict when cherry-pick directly.

#17478

Jason918 · 2022-09-07T07:53:31Z

move release/2.10.2 to #17478

Fix offload read handle npe.

03b6801

hangc0276 approved these changes Aug 11, 2022

View reviewed changes

hangc0276 requested review from codelipenghui, eolivelli, mattisonchao, michaeljmarshall and zymap August 11, 2022 03:26

hangc0276 assigned horizonzy Aug 11, 2022

hangc0276 added type/bug The PR fixed a bug or issue reported a bug release/2.10.2 release/2.9.4 release/2.8.5 labels Aug 11, 2022

hangc0276 added this to the 2.12.0 milestone Aug 11, 2022

hangc0276 added the doc-not-needed Your PR changes do not impact docs label Aug 11, 2022

codelipenghui reviewed Aug 11, 2022

View reviewed changes

add test case.

6ddb584

codelipenghui reviewed Aug 11, 2022

View reviewed changes

horizonzy added 2 commits August 11, 2022 22:48

Merge branch 'master' into fix-reset-cursor-read-slow

3cfd7e0

address comment.

2c5c03b

Technoboy- changed the title ~~Fix offload read handle npe.~~ [fix][ML] Fix offload read handle NPE. Aug 12, 2022

Technoboy- approved these changes Aug 12, 2022

View reviewed changes

make offload v2 also check closed state.

7f46a31

Technoboy- reviewed Aug 12, 2022

View reviewed changes

Comment thread .../java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImplV2.java Outdated

horizonzy added 3 commits August 12, 2022 17:33

get the slowest non-durable read position from cursors directly.

2c751e3

get the slowest non-durable read position from cursors directly.

e4b86ba

remove useless import.

02e7f78

codelipenghui approved these changes Aug 12, 2022

View reviewed changes

codelipenghui merged commit 6fc48d1 into apache:master Aug 13, 2022

Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Aug 14, 2022

[fix][ML] Fix offload read handle NPE (apache#17056)

b154e8f

Technoboy- modified the milestones: 2.12.0, 2.11.0 Aug 15, 2022

Technoboy- added the cherry-picked/branch-2.11 label Aug 15, 2022

Technoboy- pushed a commit that referenced this pull request Aug 15, 2022

[fix][ML] Fix offload read handle NPE (#17056)

57b9824

hangc0276 added the release/2.7.5 label Aug 15, 2022

codelipenghui added release/2.7.6 and removed release/2.7.5 labels Aug 15, 2022

hangc0276 pushed a commit that referenced this pull request Aug 16, 2022

[fix][ML] Fix offload read handle NPE (#17056)

f41f62d

(cherry picked from commit 6fc48d1)

zymap added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Aug 16, 2022

Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022

[fix][ML] Fix offload read handle NPE (apache#17056)

ffbc735

horizonzy added a commit to horizonzy/pulsar that referenced this pull request Sep 5, 2022

Fix offload read handle NPE.

cc51bb9

This pr cherry-pick from apache#17056.

horizonzy mentioned this pull request Sep 5, 2022

[Branch-2.10] [fix][ML] Fix offload read handle NPE. #17478

Merged

4 tasks

Jason918 removed the release/2.10.2 label Sep 7, 2022

ethqunzhong mentioned this pull request Sep 15, 2022

[Bug] Consumer trying to read a non-exist ledger #17516

Open

2 tasks

Uh oh!

Conversation

horizonzy commented Aug 11, 2022 • edited by hangc0276 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Uh oh!

hangc0276 left a comment

Choose a reason for hiding this comment

Uh oh!

hangc0276 Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

codelipenghui Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Aug 11, 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!

codelipenghui Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

codelipenghui Aug 11, 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!

codelipenghui commented Aug 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

horizonzy commented Aug 12, 2022

Uh oh!

michaeljmarshall commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jason918 commented Sep 4, 2022

Uh oh!

horizonzy commented Sep 4, 2022

Uh oh!

horizonzy commented Sep 5, 2022

Uh oh!

Jason918 commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

horizonzy commented Aug 11, 2022 •

edited by hangc0276

Loading

horizonzy Aug 11, 2022 •

edited

Loading

codelipenghui Aug 11, 2022 •

edited

Loading

michaeljmarshall commented Aug 16, 2022 •

edited

Loading